Download errors, backup files contain trailing HTML, corrupts archive backups

Are you using user 1 to access this page or a different user?

All Users.

I have this issue as well.

I am experiencing the same issue.

I have this issue as well using Drupal 9.2.7

Is there any updates with it? I'm facing with the same on our Production site but I cannot reproduce it on my local environment.
Drupal 9.1.3
Backup and Migrate 5.0.1

I'm encountering this error, but it's not related to permissions. I am using Drupal 9.2.10, backup_migrate 5.0.1. After the file is created in the /private or /tmp directory, there's an error I can't seem to back-trace. The error definitely happens after the file is generated on the server (So after /modules/contrib/backup_migrate/src/Controller/BackupController.phpBackupController::download()) All I get is this information:

http://docker.test/admin/config/development/backup_migrate/settings/destination/backups/private_files/download/backup-2021-11-30T14-37-40.mysql.gz
Referrer	http://docker.test/admin/config/development/backup_migrate/backups
Message	Symfony\Component\HttpKernel\Exception\ControllerDoesNotReturnResponseException: The controller must return a "Symfony\Component\HttpFoundation\Response" object but it returned null. Did you forget to add a return statement somewhere in your controller? in () (line 98 of /var/www/html/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php).
Severity	

If I create field with a file field that uploads to the private directory, it doesn't have any trouble downloading the file. So there's no trouble translating private files to viable downloads. The browser experience is that it shows the file being downloaded, and then after it's completed, it shows the "Failed Network Error" over the icon (in Chrome).

There's another error that occurs that I can't tell is related, but happens every time I run the download.

Notice: Undefined index: function in Drupal\Core\Utility\Error::getLastCaller() (line 125 of core/lib/Drupal/Core/Utility/Error.php).
Drupal\Core\Utility\Error::getLastCaller(Array) (Line: 56)
Drupal\Core\Utility\Error::decodeException(Object) (Line: 69)
Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber->onError(Object) (Line: 97)
Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber->onException(Object, 'kernel.exception', Object)
call_user_func(Array, Object, 'kernel.exception', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.exception') (Line: 219)
Symfony\Component\HttpKernel\HttpKernel->handleThrowable(Object, Object, 1) (Line: 91)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 717)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Notice: Undefined index: function in Drupal\Core\Utility\Error::getLastCaller() (line 125 of core/lib/Drupal/Core/Utility/Error.php).
Drupal\Core\Utility\Error::getLastCaller(Array) (Line: 56)
Drupal\Core\Utility\Error::decodeException(Object) (Line: 81)
Drupal\Core\EventSubscriber\FinalExceptionSubscriber->onException(Object, 'kernel.exception', Object)
call_user_func(Array, Object, 'kernel.exception', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.exception') (Line: 219)
Symfony\Component\HttpKernel\HttpKernel->handleThrowable(Object, Object, 1) (Line: 91)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 717)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Notice: Undefined index: function in Drupal\Core\Utility\Error::getLastCaller() (line 125 of core/lib/Drupal/Core/Utility/Error.php).
Drupal\Core\Utility\Error::getLastCaller(Array) (Line: 56)
Drupal\Core\Utility\Error::decodeException(Object) (Line: 69)
Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber->onError(Object) (Line: 97)
Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber->onException(Object, 'kernel.exception', Object)
call_user_func(Array, Object, 'kernel.exception', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.exception') (Line: 219)
Symfony\Component\HttpKernel\HttpKernel->handleThrowable(Object, Object, 1) (Line: 91)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 717)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Notice: Undefined index: function in Drupal\Core\Utility\Error::getLastCaller() (line 125 of core/lib/Drupal/Core/Utility/Error.php).
Drupal\Core\Utility\Error::getLastCaller(Array) (Line: 56)
Drupal\Core\Utility\Error::decodeException(Object) (Line: 81)
Drupal\Core\EventSubscriber\FinalExceptionSubscriber->onException(Object, 'kernel.exception', Object)
call_user_func(Array, Object, 'kernel.exception', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.exception') (Line: 219)
Symfony\Component\HttpKernel\HttpKernel->handleThrowable(Object, Object, 1) (Line: 91)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 717)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Notice: Undefined index: function in Drupal\Core\Utility\Error::getLastCaller() (line 125 of core/lib/Drupal/Core/Utility/Error.php).
Drupal\Core\Utility\Error::getLastCaller(Array) (Line: 56)
Drupal\Core\Utility\Error::decodeException(Object) (Line: 69)
Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber->onError(Object) (Line: 97)
Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber->onException(Object, 'kernel.exception', Object)
call_user_func(Array, Object, 'kernel.exception', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.exception') (Line: 219)
Symfony\Component\HttpKernel\HttpKernel->handleThrowable(Object, Object, 1) (Line: 91)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 717)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Notice: Undefined index: function in Drupal\Core\Utility\Error::getLastCaller() (line 125 of core/lib/Drupal/Core/Utility/Error.php).
Drupal\Core\Utility\Error::getLastCaller(Array) (Line: 56)
Drupal\Core\Utility\Error::decodeException(Object) (Line: 81)
Drupal\Core\EventSubscriber\FinalExceptionSubscriber->onException(Object, 'kernel.exception', Object)
call_user_func(Array, Object, 'kernel.exception', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.exception') (Line: 219)
Symfony\Component\HttpKernel\HttpKernel->handleThrowable(Object, Object, 1) (Line: 91)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 717)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I would be interested in fixing this bug if I can get some help and direction.

For me this issue was happening on our server setup on AWS with kubernettes. I did not have a problem on standard hosting server systems.

We discovered that our issue was with a broken varnish pod in our K8s cluster. Not sure if this information will help others with this problem, but it is now resolved for us.

I seem to be able to download the file from my local environment, but not a remote environment.

It looks like \Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber::wrapControllerExecutionInRenderContext() is returning NULL, so that's where I'm going to start debugging.

It looks like there is a todo in \Drupal\backup_migrate\Drupal\Destination\DrupalBrowserDownloadDestination::saveFile to refactor to use a Symfony response. It seems like that's necessary from the error message we're getting. I'm not quite sure how to do that, but I can try!

Same issue on Drupal9 with 5.0.1. We had the issue only on AWS EC2 instance and not on linode. Both servers have php 7.4

The error clearly says it is missing a required Response and the function download() is not returning any.

@chris-dart Simple patch that fixes the issue.

Thanks for providing the patch, let's see what the testbot thinks of it.

Well, the testbot thinks it is fine. But I still get "Failed - Network error" when downloading an SQL backup.

From what I can see in the partial crdownload file, the SQL file is sent, followed by a small piece of HTML with a redirect to https://site/admin/config/development/backup_migrate

This is most likely a returned RedirectResponse from the normal Drupal form handler. That would seem to invalidate the need for this code.

Reverting the change in #3180214: Site does not switch out of maintenance mode after backup finished fixes the problem and finishes the download.

For me, downloads became html files and I was getting the error

"The controller must return a "Symfony\Component\HttpFoundation\Response" object but it returned null."

As jcnventura suggested, reverting the change made in #3180214: Site does not switch out of maintenance mode after backup finished fixed the issue.

Fixed patch.

Let's expand the test coverage to make sure this problem is solved, without breaking the existing maintenance-mode functionality.

In other words, #20 means that we need to check if the module placed the site in maintenance mode before the backup and removed the maintenance mode before calling the exit() function.

And of course, we need to test it

I just did a manual test of the patch on hosting provider, and while it fixes the problem - the download is finished 100% - it leaves the website in maintenance mode. Here are the steps.

1. Visit /admin/config/development/backup_migrate/advanced
2. Check "Take site offline"
3. Press "Backup Now"

The SQL dump was correctly downloaded, but the website remains in the maintenance mode.

#19 fix problem!

The patch in #19 worked for me, too. The site did not stay in maintenance mode. All good. Thanks.

#19 works fine.

I will do some more tests.

Patch #19 is working for me. Site out of maintenance mode, run backup, backup completes and is downloaded, site is not in maintenance mode.

Thank you for the reviews of the change. Let's add some test coverage as detailed in #21.

I applied patch #19 by @jillh68 and created a simple test.

Unfortunately, the test results in Drupal still being in maintenance mode, after the backup has finished. Manually testing this will result in the correct expected behaviour... Maybe the test is missing something, I am unsure.

Setting this to "needs review", maybe someone can fix the test.

EDIT: Of course, the backup will still be in full effect, when checking maintenance mode, so instead, we should change this test to being a JS test and wait for the success message to appear, before checking maintenance mode again.

Tests were actually correct.

The site will NOT leave maintenance mode, if the backup destination is set to "Download". But it WILL leave maintenance mode, if "Private Files Directory" is set as the backup destination.

This is represented in the added test.

Adding related issue, since it was mentioned in #16 and #17 that downloaded files contain html.

After testing the MR, I can confirm that the applied changes fix the related issue with trailing HTML on downloaded backups!

I will mark that issue a duplicate of this one. Thank you for the fix!

Patch for convenience.

Bumping priority to match that of the related issue. (Backups containing trailing HTML means that backups contain data that shouldn't be there, possibly corrupting a site on restore.)

Just found a bug, while fixing the last failing test. When downloading the backup, and simultaneously having "Take site offline" checked, the site won't go online after the download has finished.

EDIT: Whoops, already mentioned that in #33.

Taking a further look, the destinations "download" (and "upload" for some reason) are added through line 144-149 in backup_migrate.module:

// @todo Make this better.
if (empty($options['nobrowser'])) {
// Add a download destination.
$user = \Drupal::currentUser();
if ($user->hasPermission('access backup files')) {
$destinations->add('download', new DrupalBrowserDownloadDestination(new Config(['name' => t('Download')])));
}
// Add an upload destination.
$destinations->add('upload', new DrupalBrowserUploadDestination(new Config(['name' => t('Upload')])));
}

I think, that is the root of the problem. Differently from the destinations "download" and "upload", the "private_files" destination is generated through an install.yml file ("backup_migrate.backup_migrate_destination.private_files.yml"). Although I don't completely understand the workflow yet.
I first thought destinations would be simply plugins, but they are not.
The PluginManager calls all plugins "teardown" method, if they implement one. But since none of these destinations are plugins, they do not implement such a method nor extend the Plugin class. Instead, the "DrupalUtils" plugin will get the site back online for the private_files destination. But it only does that for the private_files destination, even though we add the DrupalUtils plugin a few lines below of the before mentioned download and upload destinations.

No idea, we could disallow taking the site offline for both download and upload destinations as a quick fix. Everything else needs to be done by someone with further knowledge.

Should a test also be added to ensure that the related issue doesn't occur again? (Making sure that the downloaded backup is correct)

@Grevil, I think it would be okay to disable taking the site offline for download, for sure, since you can easily download the file afterwards at /admin/config/development/backup_migrate/backups.

I haven't used the upload destination, so I don't know for that one.

Don't have the full picture, since I commented / added code here a while ago, but we could skip the test using the "take site offline" setting (testAdvancedBackupWithMaintenanceModeEnabledDestinationDownload), if that is manually tested and damien is fine with that.

This is the last issue blocking the D11 compatible 5.1.x release.

If you want to finish the test @solideogloria, the current problem is, that in testAdvancedBackupWithMaintenanceModeEnabledDestinationDownload in line 511, the site is still in maintenance mode ("Current response status code is 503, but 200 expected"), maybe a race condition? Unsure as I haven't touched this in a while, maybe you find a way to fix, (or we remove the test entirely, if that is ok).

EDIT: Trash that, I misunderstood the comment by @solideogloria. As mentioned above, the test result is NOT a false positive.

@Grevil I'd say we should either revert that feature or finally fix the effects of this issue, to not block the 5.1.0 release. How to proceed?

Ok, we now throw a validation error, when the "site_offline" checkbox is checked AND the "destination_id" is set to "download". I also adjusted the tests accordingly.
I created a minor follow-up issue, where we can resolve this incompatibility in the future: #3475192: Problem with "advanced backup" when checking destination "download" and "take site offline".

@Grevil, I think it would be okay to disable taking the site offline for download, for sure, since you can easily download the file afterwards at /admin/config/development/backup_migrate/backups.

Not really a big fan of disabling the checkbox through drupal states. The "site_offline" checkbox comes before the "destination_id" select, so if the user checks the checkbox, and changes the destination he won't even see, that the checkbox is suddenly disabled and the validation error occurs. Then he has to scroll all the way down to use a different destination just so he can uncheck the checkbox again. Additionally to disabling the checkbox, we could uncheck it automatically, once the destination changes to "download" but this is even more risky IMO, as then the user might accidentally does NOT take the site in maintenance mode.

That's why I implemented it as is. 🙂

Please review!

I think it would be okay to disable taking the site offline for download, for sure, since you can easily download the file afterwards at /admin/config/development/backup_migrate/backups.

That's not true. When using Download as the destination, the backup doesn't get saved to /admin/config/development/backup_migrate/backups

@Grevil: I left some comments.

I don't think that this is a proper fix. Instead I agree with #21:

In other words, #20 means that we need to check if the module placed the site in maintenance mode before the backup and removed the maintenance mode before calling the exit() function.

And of course, we need to test it

@damienmckenna should decide, if we should go this way for now, to get 5.1.0 out and this critical one fixed and create a follow-up for the proposed solution in #21. That would be acceptable, but at least comments and @todo's should be added then, linking the follow-up.

@damienmckenna how should we proceed here?

PS: Thanks for adding the important test, that the front page is still functional.

Is there already a test available to check the resulting backup for consistency? (Not being broken like before)? Then we should also add that one for all backup tests, I think!

If not, we should have a follow-up to implement one to save us from dangerous broken / corrupted backups?
BTW I'd indeed vote to fix it like this for now and have two follow-ups to reduce the risky impact of this bug asap. But let's wait for Damien's feedback!

The exit(); is simply readded. It was originally removed through #3180214: Site does not switch out of maintenance mode after backup finished:
https://git.drupalcode.org/project/backup_migrate/-/commit/790cdc405d646...

There is already a follow-up issue, where this can be solved cleanly:
#3475192: Problem with "advanced backup" when checking destination "download" and "take site offline".

I made a comment for this!

Alright, adjusted the comments, please do a final review!

The exit(); is simply readded. It was originally removed through #3180214: Site does not switch out of maintenance mode after backup finished

Yes, that shows the importance of the comment to explain, why it should _not_ be removed.

Thanks for the fixes and the follow-up! I'm setting this RTBC to push this important fix forward, but as of #51 and #52 let's wait for Damien's final feedback and decision how to proceed.

I definitely vote for fixing this issue right now. We already implemented a temporary workaround, so that enabling "take site offline" and setting destination to "download" will throw a validation error. And we can follow up everything else in #3475192: Problem with "advanced backup" when checking destination "download" and "take site offline".

This looks really good, thank you.

A small nitpick: any URLs should be noted on separate lines with a @see prefix.

Done!

Is there already a test available to check the resulting backup for consistency? (Not being broken like before)?

I don't think so. It would be nice to have something check that the resulting backup actually works. I have occasionally found backup files that contain multiple INSERT statements with the same primary key, causing restore to fail. This could be due to site use during the creation of the backup. No test was added to ensure that the gzipped backup file is valid, either (that it doesn't contain trailing HTML, making it an invalid file).

Added follow-up. #3475585: Add CI tests to ensure created backups are valid

@solideogloria Thanks!

Nitpick from #57 solved! :)

Excellent work, thanks everyone!

Yay! 🎉

Thanks, Damien! 🙂