CLI-1778: success message immediately followed by a failure — confusing and misleading.#1988
Conversation
…ng and misleading.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1988 +/- ##
=========================================
Coverage 92.42% 92.42%
- Complexity 1956 1957 +1
=========================================
Files 123 123
Lines 7091 7091
=========================================
Hits 6554 6554
Misses 537 537 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
deepakmishra2
requested review from
anujkaushal,
kalindi-adhiya and
vishalkhode1
|
Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/1988/acli.phar |
There was a problem hiding this comment.
Pull request overview
This PR improves the pull database on-demand backup flow by preventing misleading success output on backup failure, adding an early guard for missing codebase backup download URLs, and tightening related PHPUnit assertions.
Changes:
- Add an explicit check for a missing
links.download.hrefin the codebase backup download path and throw anAcquiaCliExceptionwith a clearer message. - Refactor backup wait logic so “Database backup is ready!” is printed only after the notification completes successfully.
- Update
testPullDatabasesOnDemandFailto assert the exception and verify the success message is not printed when backup creation fails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Command/Pull/PullCommandBase.php |
Improves backup wait messaging and adds early validation for codebase backup download URL. |
tests/phpunit/src/Commands/Pull/PullDatabaseCommandTest.php |
Makes failure test assertions more precise and verifies no success message appears on failure. |
Comments suppressed due to low confidence (1)
src/Command/Pull/PullCommandBase.php:270
- In the codebase UUID download path,
$urlis never updated via theon_statscallback (that callback only runs on$acquiaCloudClient->stream()), but theRequestExceptionhandler later assumes$urlis a non-nullUriInterface(e.g., calls$url->getHost()). If the codebase download triggers an SSL-related curl errno (51/60), this can cause a fatal error while handling the exception. Consider initializing aUriInterfacefrom the download URL (or otherwise ensuring the SSL-fallback logic doesn't depend on$urlbeing set) before calling the HTTP client request.
if (!isset($backupResponse->links->download->href)) {
throw new AcquiaCliException('Cloud API failed to provide a valid backup download URL. The backup may have failed on the server.');
}
$downloadUrl = $backupResponse->links->download->href;
$this->httpClient->request('GET', $downloadUrl, [
'progress' => static function (mixed $totalBytes, mixed $downloadedBytes) use (&$progress, $output): void {
self::displayDownloadProgress($totalBytes, $downloadedBytes, $progress, $output);
},
'sink' => $localFilepath,
]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $this->output->writeln('<info>Database backup is ready!</info>'); | ||
| }; | ||
| $success = $this->waitForNotificationToComplete($acquiaCloudClient, $notificationUuid, $spinnerMessage, $successCallback); | ||
| Loop::run(); |
There was a problem hiding this comment.
@deepakmishra2 Why did we remove Loop::run(). How will now loop run if db update takes more time ?
There was a problem hiding this comment.
The old code in PullCommandBase had two calls that started the event loop:
- LoopHelper::getLoopy(...) — which internally calls Loop::run() at line 62. This is the blocking poll loop that checks the Cloud API notification status every 5 seconds (with a 45-minute watchdog timeout).
- Loop::run() — called again right after getLoopy() returned in PullCommandBase.
The second Loop::run() was redundant. By the time getLoopy() returns, the event loop has already run to completion — all timers were cancelled inside cancelTimers(), so the loop had no more work and exited. Calling Loop::run() again just immediately returned with nothing to do.
There was a problem hiding this comment.
LoopHelper::getLoopy() still handles the entire polling lifecycle internally via Loop::run() on line 62. If the backup takes a long time, it will keep polling every 5 seconds up to the 45-minute timeout, same as before.
| if (!$success) { | ||
| throw new AcquiaCliException('Cloud API failed to create a backup'); | ||
| } | ||
| $this->output->writeln(''); |
There was a problem hiding this comment.
@deepakmishra2 Removing Loop::run() is Ok, but we should remove this and instead use the callback function which was there earlier, instead of empty callback.
There was a problem hiding this comment.
Ohh actually it was left intentionally because to provide a blank line after the spinner finishes. This was also there earlier on line 399
|
worked for me.. |
This pull request improves error handling and user feedback in the database backup and pull process, and updates related tests for better reliability. The main changes ensure that missing backup download URLs are detected early, user messaging is clearer, and test assertions are more robust.
Error handling improvements:
AcquiaCliExceptionwith a clear message if it is missing.User feedback and messaging:
Testing improvements:
testPullDatabasesOnDemandFailtest to use a try/catch block for more precise exception assertion, and added a check to ensure the success message is not shown when the backup fails.Code cleanup:
React\EventLoop\LoopfromPullCommandBase.php.