From 3e87b826f6420d92c7489c4cc236985d4d7de125 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 12 Oct 2026 16:41:55 +0400 Subject: [PATCH] fix(downloader): remove the partial file of a failed download The cleanup unlinked `SplFileObject::getPath()`, which is the temp directory, so every release DLoad fell back from left its asset file behind. The handle is dropped before the delete so the name is freed at once on Windows filesystems without POSIX delete semantics. Assisted-By: Claude Opus 5.5 --- src/Module/Downloader/Downloader.php | 42 ++++++++++--------- .../Unit/Module/Downloader/DownloaderTest.php | 17 ++++++++ 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/Module/Downloader/Downloader.php b/src/Module/Downloader/Downloader.php index be037c5..a8554c8 100644 --- a/src/Module/Downloader/Downloader.php +++ b/src/Module/Downloader/Downloader.php @@ -507,26 +507,30 @@ private function processAsset(DownloadContext $context): \Closure $this->logger->info('Downloading into %s', (string) $temp); - await(coroutine( - (static function () use ($context, $file): void { - $generator = $context->asset->download( - // static fn(int $dlNow, int $dlSize, array $info): mixed => ($context->onProgress)( - // new Progress( - // total: $dlSize, - // current: $dlNow, - // message: 'downloading...', - // ), - // ), - ); - - foreach ($generator as $chunk) { - $file->fwrite($chunk); - } - }), - )->then(null, static function (\Throwable $e) use ($file): void { - @\unlink($file->getPath()); + try { + await(coroutine( + (static function () use ($context, $file): void { + $generator = $context->asset->download( + // static fn(int $dlNow, int $dlSize, array $info): mixed => ($context->onProgress)( + // new Progress( + // total: $dlSize, + // current: $dlNow, + // message: 'downloading...', + // ), + // ), + ); + + foreach ($generator as $chunk) { + $file->fwrite($chunk); + } + }), + )); + } catch (\Throwable $e) { + // Released before the delete: on Windows a file deleted while open can keep its name until the handle closes + $file = null; + @\unlink((string) $temp); throw $e; - })); + } return $context->file = $file; }; diff --git a/tests/Unit/Module/Downloader/DownloaderTest.php b/tests/Unit/Module/Downloader/DownloaderTest.php index 66cd69f..0b5511c 100644 --- a/tests/Unit/Module/Downloader/DownloaderTest.php +++ b/tests/Unit/Module/Downloader/DownloaderTest.php @@ -265,6 +265,23 @@ public function releaseFailingForOtherReasonsIsNotForgotten(): void } } + #[Test] + public function failedDownloadLeavesNoFileInTheTempDirectory(): void + { + $repository = self::repoWithReleases( + [ + 'v3.4.0' => ['roadrunner-3.4.0-linux-amd64.tar.gz'], + 'v3.2.0' => ['roadrunner-3.2.0-linux-amd64.tar.gz'], + ], + ['roadrunner-3.4.0-linux-amd64.tar.gz' => new \RuntimeException('Connection reset by peer')], + ); + + $result = $this->run([$repository], self::software()); + + Assert::same($result->version->string, 'v3.2.0'); + Assert::false(\file_exists($this->tempDir . '/roadrunner-3.4.0-linux-amd64.tar.gz')); + } + #[DataProvider('provideGradualStrategies')] #[Test] public function gradualFilteringSelectsAssetByEachStrategy(?OperatingSystem $os, ?Architecture $arch): void