From 8410991449c1c5a7c2ce34a2df86d4379a0180bb Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Mon, 29 Jun 2026 22:08:01 +0530 Subject: [PATCH 1/5] fix(backup): clean up staging artifacts and tighten disk pre-check 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/) 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. --- src/helper/Site_Backup_Restore.php | 166 ++++++++++++++++++++++++++--- 1 file changed, 151 insertions(+), 15 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index a287b639..93b31fff 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -51,8 +51,20 @@ class Site_Backup_Restore { // Global backup lock handle for serializing backups private $global_backup_lock_handle = null; + // Local staging dir (EE_BACKUP_DIR/) holding the in-progress archive + // (backup) or the downloaded archive (restore). Cleared once the success path + // removes it; the shutdown handler purges it only when an error/crash left it + // behind, so a failed run can't fill the disk or leave a stale partial download. + private $staging_dir = null; + + // Temp SQL query files written into the live web root, run, then removed. Each + // is removed inline on the success path; the shutdown handler is a safety net + // for an interrupt between write and remove. + private $temp_query_files = []; + public function __construct() { $this->fs = new Filesystem(); + register_shutdown_function( [ $this, 'cleanup_temp_query_files' ] ); } public function backup( $args, $assoc_args = [] ) { @@ -127,6 +139,11 @@ public function backup( $args, $assoc_args = [] ) { $this->pre_backup_check(); $backup_dir = EE_BACKUP_DIR . '/' . $this->site_data['site_url']; + // Track the staging dir so an abnormal exit (error, crash, OOM, Ctrl-C) + // purges the half-built archive instead of leaving it to fill the disk. + $this->staging_dir = $backup_dir; + register_shutdown_function( [ $this, 'cleanup_staging_dir' ] ); + $this->fs->remove( $backup_dir ); $this->fs->mkdir( $backup_dir ); @@ -151,6 +168,7 @@ public function backup( $args, $assoc_args = [] ) { $this->rclone_upload( $backup_dir ); $this->fs->remove( $backup_dir ); + $this->staging_dir = null; // Removed cleanly; nothing for the shutdown handler to do. $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); @@ -265,6 +283,11 @@ public function restore( $args, $assoc_args = [] ) { $backup_id = \EE\Utils\get_flag_value( $assoc_args, 'id' ); $backup_dir = EE_BACKUP_DIR . '/' . $this->site_data['site_url']; + // Track the staging dir so an abnormal exit purges the downloaded archive + // instead of leaving a stale partial download that a later run might reuse. + $this->staging_dir = $backup_dir; + register_shutdown_function( [ $this, 'cleanup_staging_dir' ] ); + if ( ! $this->fs->exists( $backup_dir ) ) { $this->fs->mkdir( $backup_dir ); } @@ -296,6 +319,7 @@ public function restore( $args, $assoc_args = [] ) { $this->maybe_restore_custom_docker_compose( $backup_dir ); $this->fs->remove( $backup_dir ); + $this->staging_dir = null; // Removed cleanly; nothing for the shutdown handler to do. EE::log( 'Reloading site.' ); EE::run_command( [ 'site', 'reload', $this->site_data['site_url'] ], [], [] ); @@ -342,10 +366,13 @@ private function backup_site_details( $backup_dir ) { $query = 'SELECT COUNT(*) FROM ' . $table_prefix . 'posts WHERE post_type = "attachment"'; $query_file = EE_ROOT_DIR . '/sites/' . $this->site_data['site_url'] . '/app/htdocs/query.sql'; + // Track for shutdown cleanup in case an interrupt hits before the remove below. + $this->temp_query_files[] = $query_file; $this->fs->dumpFile( $query_file, $query ); $upload_count = $this->run_wp_cli_command( 'db query < /var/www/htdocs/query.sql --skip-column-names | tr -d \'[:space:]\'', true ); $upload_count = empty( $upload_count ) ? 0 : $upload_count; $this->fs->remove( $query_file ); + $this->temp_query_files = array_values( array_diff( $this->temp_query_files, [ $query_file ] ) ); $plugin_count = $this->run_wp_cli_command( 'plugin list --format=count' ); // if it is not a number, then make it - @@ -961,26 +988,62 @@ private function pre_backup_restore_checks() { private function pre_backup_check() { $this->pre_backup_restore_checks(); - $site_path = EE_ROOT_DIR . '/sites/' . $this->site_data['site_url'] . '/app/htdocs'; - $site_size = $this->dir_size( $site_path ); + $site_root = EE_ROOT_DIR . '/sites/' . $this->site_data['site_url']; + + if ( ! $this->fs->exists( $site_root . '/app' ) ) { + $this->capture_error( + sprintf( 'Site app directory not found: %s/app', $site_root ), + self::ERROR_TYPE_FILESYSTEM, + 3003 + ); + $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + EE::error( "Site app directory does not exist: $site_root/app" ); + } + + // Size everything actually archived, not just app/htdocs: every site type + // archives the whole app/ dir, and all types also archive config/ (nginx + // for all, php for php/wp). The previous estimate sized only app/htdocs and + // so missed the rest of app/ and the config archive entirely. + $db_size = 0; + $site_size = $this->dir_size( $site_root . '/app' ); + $site_size += $this->dir_size( $site_root . '/config' ); - EE::debug( 'Site size: ' . $site_size ); + EE::debug( 'Site size (files): ' . $site_size ); if ( in_array( $this->site_data['site_type'], [ 'php', 'wp' ] ) && ! empty( $this->site_data['db_name'] ) ) { - $site_size += $this->get_db_size(); + $db_size = $this->get_db_size(); + $site_size += $db_size; EE::debug( 'Site size with db: ' . $site_size ); } + // Headroom for the transient uncompressed SQL dump (written into the web + // root, then moved into the staging dir -- briefly on disk twice when + // EE_ROOT_DIR and EE_BACKUP_DIR share a filesystem) and the growing + // archive. Requiring free space >= the full uncompressed source already + // covers the compressed archive (7z output <= input); add the dump size + // again plus a 10% slack so a near-full disk fails the check up front + // instead of part-way through the archive. + $required_size = (int) ceil( ( $site_size + $db_size ) * 1.1 ); + $free_space = disk_free_space( EE_BACKUP_DIR ); - EE::debug( 'Free space: ' . $free_space ); + if ( false === $free_space ) { + $this->capture_error( + 'Unable to determine free disk space for backup directory', + self::ERROR_TYPE_FILESYSTEM, + 3004 + ); + $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + EE::error( 'Unable to determine free disk space for backup directory.' ); + } + EE::debug( 'Required space (with headroom): ' . $required_size . ', Free space: ' . $free_space ); - if ( $site_size > $free_space ) { - $error_message = $this->build_disk_space_error_message( 'backup', $site_size, $free_space ); + if ( $required_size > $free_space ) { + $error_message = $this->build_disk_space_error_message( 'backup', $required_size, $free_space ); $this->capture_error( sprintf( 'Insufficient disk space for backup. Required: %s, Available: %s', - $this->format_bytes( $site_size ), + $this->format_bytes( $required_size ), $this->format_bytes( $free_space ) ), self::ERROR_TYPE_DISK_SPACE, @@ -1160,22 +1223,41 @@ private function format_bytes( $bytes, $precision = 2 ) { return round( $size, $precision ) . ' ' . $units[ $pow ]; } + /** + * Sum the byte size of every file under $directory. + * + * A missing directory returns 0 (some archived dirs are optional, e.g. a + * site's config/ may not exist yet) rather than aborting. Files whose size + * can't be read are counted as 0 but raise a warning so the disk-space + * pre-check isn't silently under-estimating. + */ private function dir_size( string $directory ) { $size = 0; EE::debug( "Calculating size of $directory" ); if ( ! $this->fs->exists( $directory ) ) { - EE::error( "Directory does not exist: $directory" ); + return 0; } - $files = new \RecursiveIteratorIterator( new \RecursiveDirectoryIterator( $directory, \FilesystemIterator::SKIP_DOTS ) ); + $files = new \RecursiveIteratorIterator( new \RecursiveDirectoryIterator( $directory, \FilesystemIterator::SKIP_DOTS ) ); + $unreadable = 0; foreach ( $files as $file ) { if ( ! $file->isReadable() ) { + $unreadable++; continue; } - $size += $file->getSize(); + $file_size = $file->getSize(); + if ( false === $file_size ) { + $unreadable++; + continue; + } + $size += $file_size; + } + + if ( $unreadable > 0 ) { + EE::warning( sprintf( 'Could not size %d file(s) under %s; disk-space estimate may be low.', $unreadable, $directory ) ); } EE::debug( "Size of $directory: $size" ); @@ -1204,6 +1286,8 @@ private function get_db_size() { $query_file = EE_ROOT_DIR . '/sites/' . $this->site_data['site_url'] . '/app/htdocs/db_size_query.sql'; + // Track for shutdown cleanup in case an interrupt hits before the remove below. + $this->temp_query_files[] = $query_file; $this->fs->dumpFile( $query_file, $query ); @@ -1213,14 +1297,25 @@ private function get_db_size() { $this->fs->remove( $query_file ); + $this->temp_query_files = array_values( array_diff( $this->temp_query_files, [ $query_file ] ) ); $size = 0; + $size_found = false; $size_output = explode( "\n", $output->stdout ); if ( count( $size_output ) > 1 ) { $size_array = explode( "\t", $size_output[1] ); - $size = isset( $size_array[1] ) ? $size_array[1] : 0; + if ( isset( $size_array[1] ) && is_numeric( $size_array[1] ) ) { + $size = $size_array[1]; + $size_found = true; + } + } + + // A backup proceeds even if the DB size can't be read, but warn so the + // disk-space pre-check isn't silently treating an unknown DB as 0 bytes. + if ( ! $size_found ) { + EE::warning( 'Could not determine database size; disk-space estimate may be low.' ); } EE::debug( "DB size: $size" ); @@ -1241,7 +1336,10 @@ private function list_remote_backups( $return = false ) { return []; } - $backups = explode( PHP_EOL, trim( $output->stdout ) ); // Remove extra whitespace and split + // trim()+explode() on empty output yields [''] (one empty element), so + // filter blank lines before the empty() check below -- otherwise the + // "No remote backups found" branch is unreachable for an empty listing. + $backups = array_filter( array_map( 'trim', explode( PHP_EOL, $output->stdout ) ), 'strlen' ); if ( empty( $backups ) ) { if ( ! $return ) { @@ -1598,8 +1696,10 @@ private function cleanup_old_backups() { return; } - // Check if we have more backups than allowed - if ( count( $backups ) > ( $no_of_backups + 1 ) ) { + // Check if we have more backups than allowed. array_slice() below keeps the + // first $no_of_backups, so trigger as soon as the count exceeds that (the + // previous `+ 1` retained N+1). + if ( count( $backups ) > $no_of_backups ) { $backups_to_delete = array_slice( $backups, $no_of_backups ); EE::log( sprintf( 'Cleaning up old backups. Keeping %d most recent backups.', $no_of_backups ) ); @@ -1963,4 +2063,40 @@ public function release_global_backup_lock() { EE::debug( 'Released global backup lock' ); } } + + /** + * Remove the local staging dir on abnormal exit. + * + * The success path removes it and clears $this->staging_dir, so this only + * fires when an error/crash left a half-built archive (backup) or a partial + * download (restore) behind. Idempotent and a no-op once cleared. + * + * @return void + */ + public function cleanup_staging_dir() { + if ( ! empty( $this->staging_dir ) && $this->fs->exists( $this->staging_dir ) ) { + $this->fs->remove( $this->staging_dir ); + EE::debug( 'Cleaned up staging dir after abnormal exit: ' . $this->staging_dir ); + } + $this->staging_dir = null; + } + + /** + * Remove any temp SQL query files written into the live web root. + * + * Safety net for an interrupt between a query file being written and its + * inline removal; the success path removes them and clears the list, so this + * is normally a no-op. Idempotent. + * + * @return void + */ + public function cleanup_temp_query_files() { + foreach ( $this->temp_query_files as $query_file ) { + if ( $this->fs->exists( $query_file ) ) { + $this->fs->remove( $query_file ); + EE::debug( 'Cleaned up leftover query file: ' . $query_file ); + } + } + $this->temp_query_files = []; + } } From 66e7ca0662015f4475da5dcb4064bfd48428957c Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Mon, 29 Jun 2026 22:27:41 +0530 Subject: [PATCH 2/5] fix(backup): harden cleanup ordering, shutdown safety and disk estimate 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. --- src/helper/Site_Backup_Restore.php | 74 +++++++++++++++++++----------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 93b31fff..e0bfd415 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -283,15 +283,6 @@ public function restore( $args, $assoc_args = [] ) { $backup_id = \EE\Utils\get_flag_value( $assoc_args, 'id' ); $backup_dir = EE_BACKUP_DIR . '/' . $this->site_data['site_url']; - // Track the staging dir so an abnormal exit purges the downloaded archive - // instead of leaving a stale partial download that a later run might reuse. - $this->staging_dir = $backup_dir; - register_shutdown_function( [ $this, 'cleanup_staging_dir' ] ); - - if ( ! $this->fs->exists( $backup_dir ) ) { - $this->fs->mkdir( $backup_dir ); - } - if ( $backup_id ) { // verify_backup_id() lists remote backups (rclone lsf) before the @@ -309,6 +300,17 @@ public function restore( $args, $assoc_args = [] ) { $this->pre_restore_check(); + // Track the staging dir for shutdown cleanup only AFTER pre_restore_check() + // has acquired this site's lock. Setting it earlier would let an early exit + // (e.g. invalid backup id, lock held by another process) delete a dir that a + // concurrent backup/restore of the same site is actively writing into. + $this->staging_dir = $backup_dir; + register_shutdown_function( [ $this, 'cleanup_staging_dir' ] ); + + if ( ! $this->fs->exists( $backup_dir ) ) { + $this->fs->mkdir( $backup_dir ); + } + if ( 'wp' === $this->site_data['site_type'] ) { $this->restore_wp( $backup_dir ); } else { @@ -1016,14 +1018,13 @@ private function pre_backup_check() { EE::debug( 'Site size with db: ' . $site_size ); } - // Headroom for the transient uncompressed SQL dump (written into the web - // root, then moved into the staging dir -- briefly on disk twice when - // EE_ROOT_DIR and EE_BACKUP_DIR share a filesystem) and the growing - // archive. Requiring free space >= the full uncompressed source already - // covers the compressed archive (7z output <= input); add the dump size - // again plus a 10% slack so a near-full disk fails the check up front - // instead of part-way through the archive. - $required_size = (int) ceil( ( $site_size + $db_size ) * 1.1 ); + // Require free space >= the full uncompressed source ($site_size already + // includes the DB) plus 10% slack. The compressed archive is bounded by the + // source (7z output <= input), and the transient uncompressed SQL dump is a + // copy of the DB that already lives in $site_size, so a flat 10% headroom + // for filesystem slack / archive overhead is enough -- without re-adding the + // DB a second time, which would false-reject DB-heavy sites. + $required_size = (int) ceil( $site_size * 1.1 ); $free_space = disk_free_space( EE_BACKUP_DIR ); if ( false === $free_space ) { @@ -1229,7 +1230,10 @@ private function format_bytes( $bytes, $precision = 2 ) { * A missing directory returns 0 (some archived dirs are optional, e.g. a * site's config/ may not exist yet) rather than aborting. Files whose size * can't be read are counted as 0 but raise a warning so the disk-space - * pre-check isn't silently under-estimating. + * pre-check isn't silently under-estimating. CATCH_GET_CHILD makes an + * untraversable subdirectory (e.g. permission denied) be skipped rather than + * throw UnexpectedValueException and abort the whole backup; such a subtree is + * simply not counted (the estimate may then be low). */ private function dir_size( string $directory ) { $size = 0; @@ -1240,8 +1244,12 @@ private function dir_size( string $directory ) { return 0; } - $files = new \RecursiveIteratorIterator( new \RecursiveDirectoryIterator( $directory, \FilesystemIterator::SKIP_DOTS ) ); - $unreadable = 0; + $files = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator( $directory, \FilesystemIterator::SKIP_DOTS ), + \RecursiveIteratorIterator::LEAVES_ONLY, + \RecursiveIteratorIterator::CATCH_GET_CHILD + ); + $unreadable = 0; foreach ( $files as $file ) { if ( ! $file->isReadable() ) { @@ -2074,9 +2082,16 @@ public function release_global_backup_lock() { * @return void */ public function cleanup_staging_dir() { - if ( ! empty( $this->staging_dir ) && $this->fs->exists( $this->staging_dir ) ) { - $this->fs->remove( $this->staging_dir ); - EE::debug( 'Cleaned up staging dir after abnormal exit: ' . $this->staging_dir ); + // Never throw from a shutdown safety net: Filesystem::remove() raises + // IOException on a failed unlink (e.g. permission denied), and a throw here + // would abort the remaining shutdown handlers (lock release, dash callback). + try { + if ( ! empty( $this->staging_dir ) && $this->fs->exists( $this->staging_dir ) ) { + $this->fs->remove( $this->staging_dir ); + EE::debug( 'Cleaned up staging dir after abnormal exit: ' . $this->staging_dir ); + } + } catch ( \Throwable $e ) { + EE::debug( 'Could not clean up staging dir ' . $this->staging_dir . ': ' . $e->getMessage() ); } $this->staging_dir = null; } @@ -2091,10 +2106,17 @@ public function cleanup_staging_dir() { * @return void */ public function cleanup_temp_query_files() { + // First-registered shutdown handler: must never throw, or the remaining + // handlers (staging cleanup, lock release, dash callback) would be skipped. + // Filesystem::remove() raises IOException on a failed unlink. foreach ( $this->temp_query_files as $query_file ) { - if ( $this->fs->exists( $query_file ) ) { - $this->fs->remove( $query_file ); - EE::debug( 'Cleaned up leftover query file: ' . $query_file ); + try { + if ( $this->fs->exists( $query_file ) ) { + $this->fs->remove( $query_file ); + EE::debug( 'Cleaned up leftover query file: ' . $query_file ); + } + } catch ( \Throwable $e ) { + EE::debug( 'Could not clean up query file ' . $query_file . ': ' . $e->getMessage() ); } } $this->temp_query_files = []; From e92c35627bf88dea4926f3f09f56eef10504a9a6 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Mon, 29 Jun 2026 23:28:30 +0530 Subject: [PATCH 3/5] fix(backup): guard per-site lock removal and drop a redundant temp 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. --- src/helper/Site_Backup_Restore.php | 31 ++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index e0bfd415..995f4ff6 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -170,7 +170,7 @@ public function backup( $args, $assoc_args = [] ) { $this->fs->remove( $backup_dir ); $this->staging_dir = null; // Removed cleanly; nothing for the shutdown handler to do. - $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + $this->try_remove_site_lock(); // Mark backup as completed and send success callback $this->dash_backup_completed = true; @@ -326,7 +326,7 @@ public function restore( $args, $assoc_args = [] ) { EE::log( 'Reloading site.' ); EE::run_command( [ 'site', 'reload', $this->site_data['site_url'] ], [], [] ); - $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + $this->try_remove_site_lock(); EE::success( 'Site restored successfully.' ); @@ -998,7 +998,7 @@ private function pre_backup_check() { self::ERROR_TYPE_FILESYSTEM, 3003 ); - $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + $this->try_remove_site_lock(); EE::error( "Site app directory does not exist: $site_root/app" ); } @@ -1006,15 +1006,13 @@ private function pre_backup_check() { // archives the whole app/ dir, and all types also archive config/ (nginx // for all, php for php/wp). The previous estimate sized only app/htdocs and // so missed the rest of app/ and the config archive entirely. - $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 ); if ( in_array( $this->site_data['site_type'], [ 'php', 'wp' ] ) && ! empty( $this->site_data['db_name'] ) ) { - $db_size = $this->get_db_size(); - $site_size += $db_size; + $site_size += $this->get_db_size(); EE::debug( 'Site size with db: ' . $site_size ); } @@ -1033,7 +1031,7 @@ private function pre_backup_check() { self::ERROR_TYPE_FILESYSTEM, 3004 ); - $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + $this->try_remove_site_lock(); EE::error( 'Unable to determine free disk space for backup directory.' ); } EE::debug( 'Required space (with headroom): ' . $required_size . ', Free space: ' . $free_space ); @@ -1051,11 +1049,28 @@ private function pre_backup_check() { 3001 ); - $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + $this->try_remove_site_lock(); EE::error( $error_message ); } } + /** + * Best-effort removal of the per-site lock file. + * + * Filesystem::remove() throws on a failed unlink; callers on an error path are + * about to EE::error(), so a lock-cleanup failure must not throw and mask the + * real cause. Swallow any failure (logged at debug level). + * + * @return void + */ + private function try_remove_site_lock() { + try { + $this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' ); + } catch ( \Throwable $e ) { + EE::debug( 'Could not remove site lock: ' . $e->getMessage() ); + } + } + /** * Build a disk space error message for backup/restore operations. * From f372636f11b9aad1471474cbf7c6ad0048e3f2db Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 10:43:51 +0000 Subject: [PATCH 4/5] fix(backup): cover restore download in staging cleanup and run it before 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/, 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. --- src/helper/Site_Backup_Restore.php | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 995f4ff6..b2184a85 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -65,6 +65,9 @@ class Site_Backup_Restore { public function __construct() { $this->fs = new Filesystem(); register_shutdown_function( [ $this, 'cleanup_temp_query_files' ] ); + // Registered here so it runs before any lock-release shutdown handler: once the + // site lock is released, another run may already be writing into this dir. + register_shutdown_function( [ $this, 'cleanup_staging_dir' ] ); } public function backup( $args, $assoc_args = [] ) { @@ -139,10 +142,9 @@ public function backup( $args, $assoc_args = [] ) { $this->pre_backup_check(); $backup_dir = EE_BACKUP_DIR . '/' . $this->site_data['site_url']; - // Track the staging dir so an abnormal exit (error, crash, OOM, Ctrl-C) - // purges the half-built archive instead of leaving it to fill the disk. + // Track the staging dir so a failed run (any EE::error/exit) purges the + // half-built archive instead of leaving it to fill the disk. $this->staging_dir = $backup_dir; - register_shutdown_function( [ $this, 'cleanup_staging_dir' ] ); $this->fs->remove( $backup_dir ); $this->fs->mkdir( $backup_dir ); @@ -298,19 +300,9 @@ public function restore( $args, $assoc_args = [] ) { $this->rclone_config_path = \EE\Utils\trailingslashit( $this->get_rclone_config_path() ) . $backup_id; } + // Arms staging-dir cleanup once this site's lock is held (see pre_restore_check()). $this->pre_restore_check(); - // Track the staging dir for shutdown cleanup only AFTER pre_restore_check() - // has acquired this site's lock. Setting it earlier would let an early exit - // (e.g. invalid backup id, lock held by another process) delete a dir that a - // concurrent backup/restore of the same site is actively writing into. - $this->staging_dir = $backup_dir; - register_shutdown_function( [ $this, 'cleanup_staging_dir' ] ); - - if ( ! $this->fs->exists( $backup_dir ) ) { - $this->fs->mkdir( $backup_dir ); - } - if ( 'wp' === $this->site_data['site_type'] ) { $this->restore_wp( $backup_dir ); } else { @@ -1133,6 +1125,12 @@ private function pre_restore_check() { $this->pre_backup_restore_checks(); + // Only now that this site's lock is held: track the staging dir so a failed + // download or metadata check is cleaned up, and drop any leftover from an + // earlier run (e.g. SIGKILL) so a stale or partial archive is never reused. + $this->staging_dir = EE_BACKUP_DIR . '/' . $this->site_data['site_url']; + $this->fs->remove( $this->staging_dir ); + $remote_path = $this->get_remote_path( false ); $command = sprintf( 'rclone size --json %s', escapeshellarg( $remote_path ) ); $output = EE::launch( $command ); From dd0a9e77980d1560349a5e65e52f9c138f767d03 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 10:43:51 +0000 Subject: [PATCH 5/5] fix(backup): handle getSize() failures and skip broken symlinks in dir_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. --- src/helper/Site_Backup_Restore.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index b2184a85..2b96e913 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -1265,16 +1265,20 @@ private function dir_size( string $directory ) { $unreadable = 0; foreach ( $files as $file ) { + // A broken symlink has no content to archive; don't warn about it. + if ( $file->isLink() && ! file_exists( $file->getPathname() ) ) { + continue; + } if ( ! $file->isReadable() ) { $unreadable++; continue; } - $file_size = $file->getSize(); - if ( false === $file_size ) { + // getSize() throws (never returns false) when stat fails, e.g. a file removed mid-scan. + try { + $size += $file->getSize(); + } catch ( \RuntimeException $e ) { $unreadable++; - continue; } - $size += $file_size; } if ( $unreadable > 0 ) {