diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 2eafddb4b..162912ab4 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -70,6 +70,7 @@ jobs: - { name: "SummitSubmissionInvitationServiceTest", filter: "--filter SummitSubmissionInvitationServiceTest" } - { name: "MemberServiceTest", filter: "--filter MemberServiceTest" } - { name: "EntityModelUnitTests", filter: "tests/Unit/Entities/" } + - { name: "MigrationUnitTests", filter: "tests/Unit/Migrations/" } - { name: "ModelUnitTests", filter: "tests/Unit/Models/" } - { name: "AuditUnitTests", filter: "tests/Unit/Audit/" } - { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" } @@ -78,7 +79,7 @@ jobs: - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" } # Named by path because no job in this matrix runs the tests/ root, only its # subdirectories - a file added there runs nowhere unless it is listed here. - - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php tests/PresentationSubmissionReopenedEmailTest.php" } + - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php tests/PresentationSubmissionReopenedEmailTest.php tests/CopyClonedMediaUploadFilesJobTest.php" } - { name: "SpeakerSubmitterPublishedFilter", filter: "tests/ProcessSpeakersEmailRequestJobTest.php tests/ProcessSubmittersEmailRequestJobTest.php tests/SpeakerRepositoryTest.php tests/SubmitterRepositoryTest.php tests/SpeakerServiceOriginalFilterTest.php tests/SubmitterServiceOriginalFilterTest.php" } - { name: "SpeakerBulkEmailChunkResume", filter: "tests/SpeakerServiceBulkSendChunkingTest.php tests/SpeakerServiceResumeSendEmailsTest.php tests/ProcessSpeakersEmailRequestJobResumeTest.php tests/ProcessSpeakersEmailRequestJobFailedHookTest.php" } - { name: "AttendeeBulkEmailChunkResume", filter: "tests/AttendeeServiceBulkSendChunkingTest.php tests/AttendeeServiceResumeSendEmailsTest.php tests/ProcessAttendeesEmailRequestJobResumeTest.php tests/ProcessAttendeesEmailRequestJobFailedHookTest.php tests/SummitAttendeeAnnouncementEmailTest.php tests/DoctrineSummitAttendeeRepositoryTest.php tests/SummitExtraQuestionsMemoizationTest.php" } diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php new file mode 100644 index 000000000..d2efa3b2a --- /dev/null +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -0,0 +1,72 @@ +source_media_upload_id, $this->target_media_upload_id)); + + $service->copyClonedMediaUploadFiles($this->source_media_upload_id, $this->target_media_upload_id, $this->filename); + } + + public function failed(\Throwable $exception): void + { + Log::error(sprintf( + "CopyClonedMediaUploadFilesJob::failed source %s target %s: %s", + $this->source_media_upload_id, $this->target_media_upload_id, $exception->getMessage() + )); + Log::error($exception); + + try { + App::make(ISummitService::class)->removeUncopiedClonedMediaUpload($this->target_media_upload_id, $this->filename); + } catch (\Throwable $ex) { + Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); + Log::error($ex); + } + } +} diff --git a/app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMediaUpload.php b/app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMediaUpload.php index 77d509d6a..319e36776 100644 --- a/app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMediaUpload.php +++ b/app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMediaUpload.php @@ -45,6 +45,30 @@ public function getClassName(){ #[ORM\Column(name: 'LegacyPathFormat', type: 'boolean')] private $legacy_path_format; + /** + * The "{summit}-{slug}/{presentation}-{slug}" folder segment of a modern-format upload, + * fixed when the row was backfilled so a later title or summit slug change cannot move it. + * @var string|null + */ + #[ORM\Column(name: 'FrozenPathSegment', type: 'string', length: 1024, nullable: true)] + private $frozen_path_segment; + + public function getFrozenPathSegment(): ?string + { + return $this->frozen_path_segment; + } + + public static function buildModernPathSegment(int $summitId, ?string $summitRawSlug, int $presentationId, ?string $presentationSlug): string + { + return sprintf( + '%s-%s/%s-%s', + $summitId, + filter_var($summitRawSlug, FILTER_SANITIZE_ENCODED), + $presentationId, + filter_var($presentationSlug, FILTER_SANITIZE_ENCODED) + ); + } + /** * @var SummitMediaUploadType */ @@ -141,13 +165,17 @@ public function getPath(string $storageType = IStorageTypesConstants::PublicType $summit = $this->getPresentation()->getSummit(); $presentation = $this->getPresentation(); - $format = $storageType == IStorageTypesConstants::PublicType ? '%s/%s/%s': '%s/'.IStorageTypesConstants::PrivateType.'/%s/%s'; + $prefix = $storageType == IStorageTypesConstants::PublicType ? + $mountingFolder : sprintf('%s/%s', $mountingFolder, IStorageTypesConstants::PrivateType); if($this->legacy_path_format) - return sprintf($format, $mountingFolder, $summit->getId(), $presentation->getId()); + return sprintf('%s/%s/%s', $prefix, $summit->getId(), $presentation->getId()); + + if(!empty($this->frozen_path_segment)) + return sprintf('%s/%s', $prefix, $this->frozen_path_segment); $presentation->generateSlug(); - return sprintf($format, $mountingFolder, sprintf("%s-%s", $summit->getId(), filter_var($summit->getRawSlug(), FILTER_SANITIZE_ENCODED)), sprintf("%s-%s", $presentation->getId(), filter_var($presentation->getSlug(), FILTER_SANITIZE_ENCODED))); + return sprintf('%s/%s', $prefix, self::buildModernPathSegment($summit->getId(), $summit->getRawSlug(), $presentation->getId(), $presentation->getSlug())); } public function __construct() diff --git a/app/Models/Foundation/Summit/Events/Presentations/Presentation.php b/app/Models/Foundation/Summit/Events/Presentations/Presentation.php index d2e5c9c78..cf1a6ff78 100644 --- a/app/Models/Foundation/Summit/Events/Presentations/Presentation.php +++ b/app/Models/Foundation/Summit/Events/Presentations/Presentation.php @@ -1389,14 +1389,20 @@ public function setSlug(string $slug): void '/^[\.\-_]+/' => '', // Remove all leading dots, dashes or underscores ]; - public function generateSlug(): void + public static function slugify(string $title): string { - if (empty($this->title)) return; - $this->slug = trim(Transliterator::utf8ToAscii($this->title)); + $slug = trim(Transliterator::utf8ToAscii($title)); foreach (self::$default_replacements as $regex => $replace) { - $this->slug = preg_replace($regex, $replace, $this->slug); + $slug = preg_replace($regex, $replace, $slug); } + return $slug; + } + + public function generateSlug(): void + { + if (empty($this->title)) return; + $this->slug = self::slugify($this->title); } diff --git a/app/Models/Foundation/Summit/PendingMediaUpload.php b/app/Models/Foundation/Summit/PendingMediaUpload.php index d9a1b1aa2..44347855d 100644 --- a/app/Models/Foundation/Summit/PendingMediaUpload.php +++ b/app/Models/Foundation/Summit/PendingMediaUpload.php @@ -31,6 +31,14 @@ class PendingMediaUpload extends SilverstripeBaseModel const STATUS_PUBLIC_STORAGE_UPLOADED = 'PublicStorageUploaded'; const STATUS_PRIVATE_STORAGE_UPLOADED = 'PrivateStorageUploaded'; + // statuses whose file is not fully written to storage yet + const IN_FLIGHT_STATUSES = [ + self::STATUS_PENDING, + self::STATUS_PROCESSING, + self::STATUS_PUBLIC_STORAGE_UPLOADED, + self::STATUS_PRIVATE_STORAGE_UPLOADED, + ]; + /** * @var int */ diff --git a/app/Models/Foundation/Summit/Repositories/IPendingMediaUploadRepository.php b/app/Models/Foundation/Summit/Repositories/IPendingMediaUploadRepository.php index 3424227f9..e0221033e 100644 --- a/app/Models/Foundation/Summit/Repositories/IPendingMediaUploadRepository.php +++ b/app/Models/Foundation/Summit/Repositories/IPendingMediaUploadRepository.php @@ -52,4 +52,20 @@ public function deleteCompletedOlderThan(int $days = 7, int $limit = 1000): int; * @return int Number of rows deleted */ public function deleteByMediaUpload(PresentationMediaUpload $mediaUpload): int; + + /** + * True if any row, in any status, references the given media upload. Every upload and + * re-upload through PresentationService creates one; a cloned media upload never has one. + * @param int $media_upload_id + * @return bool + */ + public function hasAnyForMediaUpload(int $media_upload_id): bool; + + /** + * True if a row whose file is not fully written yet (Pending, Processing, + * PublicStorageUploaded, PrivateStorageUploaded) references the given media upload. + * @param int $media_upload_id + * @return bool + */ + public function hasInFlightForMediaUpload(int $media_upload_id): bool; } diff --git a/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php b/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php index 4b5bf1181..46e13a69d 100644 --- a/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php +++ b/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php @@ -132,13 +132,39 @@ public function deleteByMediaUpload(PresentationMediaUpload $mediaUpload): int "DELETE FROM models\\summit\\PendingMediaUpload p WHERE p.media_upload = :mediaUpload AND p.status IN (:statuses)" ); $query->setParameter('mediaUpload', $mediaUpload->getId()); - $query->setParameter('statuses', [ - PendingMediaUpload::STATUS_PENDING, - PendingMediaUpload::STATUS_PROCESSING, - PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED, - PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED - ]); + $query->setParameter('statuses', PendingMediaUpload::IN_FLIGHT_STATUSES); return $query->execute(); } + + /** + * @inheritDoc + */ + public function hasAnyForMediaUpload(int $media_upload_id): bool + { + $count = $this->getEntityManager() + ->createQuery( + "SELECT COUNT(p.id) FROM models\\summit\\PendingMediaUpload p WHERE p.media_upload = :mediaUpload" + ) + ->setParameter('mediaUpload', $media_upload_id) + ->getSingleScalarResult(); + + return intval($count) > 0; + } + + /** + * @inheritDoc + */ + public function hasInFlightForMediaUpload(int $media_upload_id): bool + { + $count = $this->getEntityManager() + ->createQuery( + "SELECT COUNT(p.id) FROM models\\summit\\PendingMediaUpload p WHERE p.media_upload = :mediaUpload AND p.status IN (:statuses)" + ) + ->setParameter('mediaUpload', $media_upload_id) + ->setParameter('statuses', PendingMediaUpload::IN_FLIGHT_STATUSES) + ->getSingleScalarResult(); + + return intval($count) > 0; + } } diff --git a/app/Services/Model/ISummitService.php b/app/Services/Model/ISummitService.php index c7c9b1684..0068ad5ce 100644 --- a/app/Services/Model/ISummitService.php +++ b/app/Services/Model/ISummitService.php @@ -41,6 +41,13 @@ */ interface ISummitService { + // lifetime of the media upload storage lock (SummitService::mediaUploadStorageLockName); above + // CopyClonedMediaUploadFilesJob::$timeout, so a killed copy's lock expires on its own before + // anything waits on it long + // the key builder itself stays on SummitService, not here: it's static, and an interface + // cannot hold a static implementation + const MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME = 1300; + /** * @param Summit $summit * @param array $data @@ -557,6 +564,30 @@ public function regenerateTemporalUrlsForMediaUploads(int $summit_id):void; */ public function processPendingMediaUploads(int $max_retries = 3): array; + /** + * Copies a cloned media upload's files from the original's storage paths to the clone's, under + * the same storage lock processPendingMediaUploads takes. A clone that has been re-uploaded + * since is never written to. A leg whose source file is missing is skipped; a source missing + * on every enabled leg throws. + * + * @param int $source_media_upload_id + * @param int $target_media_upload_id + * @param string $filename the clone's filename when the copy was queued + * @throws \App\Services\Utils\Exceptions\UnacquiredLockException when the storage location is locked + * @throws \RuntimeException when the source is gone, has a re-upload in flight, or has no file + */ + public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $target_media_upload_id, string $filename): void; + + /** + * Removes a cloned media upload whose files could not be copied, so the clone never points at a + * missing file. A clone that has been re-uploaded since is kept. Removal failures are logged + * for the operator, not thrown. + * + * @param int $target_media_upload_id + * @param string $filename the clone's filename when the copy was queued + */ + public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename): void; + /** * @param Summit $summit * @param UploadedFile $csv_file diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index 7688a316d..435f367e8 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -21,6 +21,7 @@ use App\Events\SponsorServices\SummitDomainEvents; use App\Facades\ResourceServerContext; use App\Http\Utils\IFileUploader; +use App\Jobs\CopyClonedMediaUploadFilesJob; use App\Jobs\Emails\PresentationSubmissions\ImportEventSpeakerEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationModeratorNotificationEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationSpeakerNotificationEmail; @@ -31,6 +32,7 @@ use App\Jobs\ProcessRegistrationCompaniesDataImport; use App\Jobs\ProcessScheduleEntityLifeCycleEvent; use App\Jobs\SponsorServices\PublishSponsorServiceDomainEventsJob; +use App\Jobs\Utils\JobDispatcher; use App\Models\Foundation\Main\Factories\CompanyFactory; use App\Models\Foundation\Summit\Factories\LeadReportSettingsFactory; use App\Models\Foundation\Summit\Factories\PresentationFactory; @@ -54,6 +56,8 @@ use App\Services\Model\AbstractPublishService; use App\Services\Model\IMemberService; use App\Services\Model\Imp\Notifications\SpeakerChangeNotifications; +use App\Services\Utils\Exceptions\UnacquiredLockException; +use App\Services\Utils\ILockManagerService; use App\Services\Utils\Security\IEncryptionAES256KeysGenerator; use DateInterval; use DateTime; @@ -253,6 +257,19 @@ final class SummitService */ private $cache_service; + /** + * @var ILockManagerService + */ + private $lock_service; + + // Keyed by lock name, set for the duration copyClonedMediaUploadFiles holds that lock. On a + // final-attempt timeout, Laravel's Worker::registerTimeoutHandler calls the job's failed() from + // the SIGALRM handler in the SAME process while the copy is suspended mid-write still holding + // the lock - its finally never runs before the process is killed. A static, not an instance + // property, so the token reaches removeUncopiedClonedMediaUpload however the service was + // resolved; it lives as long as the process does. See removeUncopiedClonedMediaUpload. + private static array $held_media_upload_lock_tokens = []; + /** * @param ISummitRepository $summit_repository * @param ISummitEventRepository $event_repository @@ -280,6 +297,7 @@ final class SummitService * @param IEncryptionAES256KeysGenerator $encryption_key_generator * @param IMUXApi $mux_api * @param ICacheService $cache_service + * @param ILockManagerService $lock_service * @param ITransactionService $tx_service */ public function __construct @@ -310,6 +328,7 @@ public function __construct IEncryptionAES256KeysGenerator $encryption_key_generator, IMUXApi $mux_api, ICacheService $cache_service, + ILockManagerService $lock_service, ITransactionService $tx_service ) { @@ -340,6 +359,7 @@ public function __construct $this->download_strategy = $download_strategy; $this->mux_api = $mux_api; $this->cache_service = $cache_service; + $this->lock_service = $lock_service; } /** @@ -2107,7 +2127,9 @@ public function removeModeratorFromPresentation(int $current_member_id, int $spe */ public function cloneEvent(Summit $summit, int $event_id): SummitEvent { - return $this->tx_service->transaction(function () use ($summit, $event_id) { + [$event_clone, $media_upload_pairs] = $this->tx_service->transaction(function () use ($summit, $event_id) { + // built per attempt: runRootTransaction can re-run this callback after a reconnectable error + $media_upload_pairs = []; $event = $this->event_repository->getById($event_id); if (is_null($event)) @@ -2255,13 +2277,47 @@ public function cloneEvent(Summit $summit, int $event_id): SummitEvent $material_clone = $material->clone(); if ($material_clone != null) { $event_clone->addMaterial($material_clone); + if ($material instanceof PresentationMediaUpload) + $media_upload_pairs[] = [$material, $material_clone]; } } } + $this->event_repository->add($event_clone); - return $event_clone; + return [$event_clone, $media_upload_pairs]; }); + + // The clone's media upload records point at the clone's own storage path, where nothing + // exists yet. Copy the files after commit so the clone has IDs and a slow copy does not + // hold the transaction open. + foreach ($media_upload_pairs as [$source, $target]) { + $job = new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), $target->getFilename()); + try { + JobDispatcher::withDbFallback( + job: $job, + logContext: ['summit_id' => $summit->getId(), 'event_id' => $event_id, 'clone_id' => $event_clone->getId()], + primaryConnection: Config::get('queue.default') + ); + } catch (\Throwable $ex) { + // only reachable when both queues are down and the synchronous dispatch itself threw + // (Laravel's SyncQueue resolves/builds the job outside its own try, so a failure there + // never reaches handle() or failed()); run the same cleanup failed() runs so the clone's + // upload is removed instead of pointing at a file that was never copied. The cleanup is + // idempotent and logs its own errors, but guard the call anyway so cloneEvent still + // returns the already-committed clone. + Log::error(sprintf("SummitService::cloneEvent copy of media upload %s for clone %s failed: %s", $target->getId(), $event_clone->getId(), $ex->getMessage())); + Log::error($ex); + try { + $this->removeUncopiedClonedMediaUpload($target->getId(), $target->getFilename()); + } catch (\Throwable $cleanup_ex) { + Log::error(sprintf("SummitService::cloneEvent cleanup for media upload %s on clone %s threw: %s", $target->getId(), $event_clone->getId(), $cleanup_ex->getMessage())); + Log::error($cleanup_ex); + } + } + } + + return $event_clone; } /** @@ -4437,118 +4493,27 @@ public function processPendingMediaUploads(int $max_retries = 3): array continue; } + // The same storage-location lock copyClonedMediaUploadFiles holds while it writes a + // clone's files. If a copy is running, leave this row untouched for the next run, so + // the staff file is written after the copy and never overwritten by it. + $presentation = $pending_upload->getMediaUpload()->getPresentation(); + if (is_null($presentation)) { + // no presentation means no valid path to write to; leave the row untouched + Log::warning(sprintf("SummitService::processPendingMediaUploads upload ID %s skipped, media upload %s has no presentation", $upload_id, $pending_upload->getMediaUpload()->getId())); + continue; + } + $lock_name = self::mediaUploadStorageLockName($presentation->getId(), $pending_upload->getFileName()); try { - // Mark as Processing and increment attempts - $this->tx_service->transaction(function () use ($pending_upload) { - $pending_upload->setStatus(PendingMediaUpload::STATUS_PROCESSING); - $pending_upload->incrementAttempts(); - }); - - // Get summit and media upload type - $summit = $this->summit_repository->getById($pending_upload->getSummitId()); - if (is_null($summit)) { - throw new EntityNotFoundException(sprintf("Summit %s not found", $pending_upload->getSummitId())); - } - - $mediaUploadType = $summit->getMediaUploadTypeById($pending_upload->getMediaUploadTypeId()); - if (is_null($mediaUploadType)) { - throw new EntityNotFoundException(sprintf("Media Upload Type %s not found", $pending_upload->getMediaUploadTypeId())); - } - - // Always re-download temp file from upload storage (even on retry from partial status) - $localPath = self::getFileFromRemoteStorageOnTempStorage( - $pending_upload->getFileName(), - $pending_upload->getTempFilePath() - ); - - $currentStatus = $pending_upload->getStatus(); - - // Phase 1: Public storage (skip if already done) - if ($currentStatus !== PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED) { - - $publicStrategy = FileUploadStrategyFactory::build($mediaUploadType->getPublicStorageType()); - if (!is_null($publicStrategy)) { - Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s saving to public storage", $upload_id)); - try { - $options = $mediaUploadType->isUseTemporaryLinksOnPublicStorage() ? [] : 'public'; - $publicStrategy->saveFromPath( - $localPath, - $pending_upload->getPublicPath(), - $pending_upload->getFileName(), - $options - ); - } catch (\Exception $public_ex) { - // Public storage failed - re-throw to outer catch - throw $public_ex; - } - } - // Public done (or skipped because None) — commit partial status - $this->tx_service->transaction(function () use ($pending_upload) { - $pending_upload->setStatus(PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED); - }); - $currentStatus = PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED; - Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s public storage completed", $upload_id)); - } else { - Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s skipping public storage (current status: %s)", $upload_id, $currentStatus)); - } - - // Phase 2: Private storage (skip if already done) - if ($currentStatus !== PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED) { - $privateStrategy = FileUploadStrategyFactory::build($mediaUploadType->getPrivateStorageType()); - if (!is_null($privateStrategy)) { - Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s saving to private storage", $upload_id)); - try { - $privateStrategy->saveFromPath( - $localPath, - $pending_upload->getPrivatePath(), - $pending_upload->getFileName() - ); - } catch (\Exception $private_ex) { - // Private storage failed - status stays at PublicStorageUploaded for retry - throw $private_ex; - } - } - // Private done (or skipped because None) — commit partial status - $this->tx_service->transaction(function () use ($pending_upload) { - $pending_upload->setStatus(PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED); - }); - $currentStatus = PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED; - Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s private storage completed", $upload_id)); - } else { - Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s skipping private storage (current status: %s)", $upload_id, $currentStatus)); - } - - // Both phases done — mark Completed - $this->tx_service->transaction(function () use ($pending_upload) { - $pending_upload->setStatus(PendingMediaUpload::STATUS_COMPLETED); - $pending_upload->setProcessedDate(new \DateTime('now', new \DateTimeZone(\models\utils\SilverstripeBaseModel::DefaultTimeZone))); - }); - - // Clean up temp files after transaction commits successfully - self::cleanLocalAndRemoteFile($localPath, $pending_upload->getTempFilePath()); - - $stats['processed']++; - Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s completed successfully", $upload_id)); - - } catch (\Exception $ex) { - // On failure, status remains at whatever partial state was reached - // Set error message and mark as ERROR only if max retries exhausted - $this->tx_service->transaction(function () use ($pending_upload, $ex, $max_retries) { - $pending_upload->setErrorMessage($ex->getMessage()); - if ($pending_upload->getAttempts() >= $max_retries) { - $pending_upload->setStatus(PendingMediaUpload::STATUS_ERROR); - } - // else: leave at current status (Pending, PublicStorageUploaded, or PrivateStorageUploaded) for retry - }); + $lock_token = $this->lock_service->acquireLock($lock_name, ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME); + } catch (UnacquiredLockException $lock_ex) { + Log::info(sprintf("SummitService::processPendingMediaUploads upload ID %s skipped, media upload storage is locked", $upload_id)); + continue; + } - $stats['errors']++; - Log::warning(sprintf( - "SummitService::processPendingMediaUploads upload ID %s failed (attempt %s/%s): %s", - $upload_id, - $pending_upload->getAttempts(), - $max_retries, - $ex->getMessage() - )); + try { + $this->processPendingUpload($pending_upload, $upload_id, $max_retries, $stats); + } finally { + $this->lock_service->releaseLock($lock_name, $lock_token); } } @@ -4577,4 +4542,403 @@ public function processPendingMediaUploads(int $max_retries = 3): array return $stats; } + + /** + * Writes one pending upload to public then private storage. Runs under the upload's storage + * lock, which processPendingMediaUploads holds and releases. + * @param PendingMediaUpload $pending_upload + * @param int $upload_id + * @param int $max_retries + * @param array $stats + */ + private function processPendingUpload(PendingMediaUpload $pending_upload, int $upload_id, int $max_retries, array &$stats): void + { + try { + // Mark as Processing and increment attempts + $this->tx_service->transaction(function () use ($pending_upload) { + $pending_upload->setStatus(PendingMediaUpload::STATUS_PROCESSING); + $pending_upload->incrementAttempts(); + }); + + // Get summit and media upload type + $summit = $this->summit_repository->getById($pending_upload->getSummitId()); + if (is_null($summit)) { + throw new EntityNotFoundException(sprintf("Summit %s not found", $pending_upload->getSummitId())); + } + + $mediaUploadType = $summit->getMediaUploadTypeById($pending_upload->getMediaUploadTypeId()); + if (is_null($mediaUploadType)) { + throw new EntityNotFoundException(sprintf("Media Upload Type %s not found", $pending_upload->getMediaUploadTypeId())); + } + + // Always re-download temp file from upload storage (even on retry from partial status) + $localPath = self::getFileFromRemoteStorageOnTempStorage( + $pending_upload->getFileName(), + $pending_upload->getTempFilePath() + ); + + $currentStatus = $pending_upload->getStatus(); + + // Phase 1: Public storage (skip if already done) + if ($currentStatus !== PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED) { + + $publicStrategy = FileUploadStrategyFactory::build($mediaUploadType->getPublicStorageType()); + if (!is_null($publicStrategy)) { + Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s saving to public storage", $upload_id)); + try { + $options = $mediaUploadType->isUseTemporaryLinksOnPublicStorage() ? [] : 'public'; + $publicStrategy->saveFromPath( + $localPath, + $pending_upload->getPublicPath(), + $pending_upload->getFileName(), + $options + ); + } catch (\Exception $public_ex) { + // Public storage failed - re-throw to outer catch + throw $public_ex; + } + } + // Public done (or skipped because None) — commit partial status + $this->tx_service->transaction(function () use ($pending_upload) { + $pending_upload->setStatus(PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED); + }); + $currentStatus = PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED; + Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s public storage completed", $upload_id)); + } else { + Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s skipping public storage (current status: %s)", $upload_id, $currentStatus)); + } + + // Phase 2: Private storage (skip if already done) + if ($currentStatus !== PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED) { + $privateStrategy = FileUploadStrategyFactory::build($mediaUploadType->getPrivateStorageType()); + if (!is_null($privateStrategy)) { + Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s saving to private storage", $upload_id)); + try { + $privateStrategy->saveFromPath( + $localPath, + $pending_upload->getPrivatePath(), + $pending_upload->getFileName() + ); + } catch (\Exception $private_ex) { + // Private storage failed - status stays at PublicStorageUploaded for retry + throw $private_ex; + } + } + // Private done (or skipped because None) — commit partial status + $this->tx_service->transaction(function () use ($pending_upload) { + $pending_upload->setStatus(PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED); + }); + $currentStatus = PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED; + Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s private storage completed", $upload_id)); + } else { + Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s skipping private storage (current status: %s)", $upload_id, $currentStatus)); + } + + // Both phases done — mark Completed + $this->tx_service->transaction(function () use ($pending_upload) { + $pending_upload->setStatus(PendingMediaUpload::STATUS_COMPLETED); + $pending_upload->setProcessedDate(new \DateTime('now', new \DateTimeZone(\models\utils\SilverstripeBaseModel::DefaultTimeZone))); + }); + + // Clean up temp files after transaction commits successfully + self::cleanLocalAndRemoteFile($localPath, $pending_upload->getTempFilePath()); + + $stats['processed']++; + Log::debug(sprintf("SummitService::processPendingMediaUploads upload ID %s completed successfully", $upload_id)); + + } catch (\Exception $ex) { + // On failure, status remains at whatever partial state was reached + // Set error message and mark as ERROR only if max retries exhausted + $this->tx_service->transaction(function () use ($pending_upload, $ex, $max_retries) { + $pending_upload->setErrorMessage($ex->getMessage()); + if ($pending_upload->getAttempts() >= $max_retries) { + $pending_upload->setStatus(PendingMediaUpload::STATUS_ERROR); + } + // else: leave at current status (Pending, PublicStorageUploaded, or PrivateStorageUploaded) for retry + }); + + $stats['errors']++; + Log::warning(sprintf( + "SummitService::processPendingMediaUploads upload ID %s failed (attempt %s/%s): %s", + $upload_id, + $pending_upload->getAttempts(), + $max_retries, + $ex->getMessage() + )); + } + } + + /** + * The storage lock shared by processPendingMediaUploads and copyClonedMediaUploadFiles. Keyed on + * the storage location (presentation + filename), not the upload ID: summit-admin re-uploads by + * delete-then-add, which gives the new file a new upload ID at the same path. The filename is + * hashed so an arbitrary filename is always a safe cache key. + * @param int $presentation_id + * @param string $filename + * @return string + */ + public static function mediaUploadStorageLockName(int $presentation_id, string $filename): string + { + return sprintf('media_upload_storage.%s.%s.lock', $presentation_id, sha1($filename)); + } + + /** + * @inheritDoc + */ + public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $target_media_upload_id, string $filename): void + { + $lock_name = $this->resolveClonedMediaUploadLockName($target_media_upload_id, $filename); + if (is_null($lock_name)) { + Log::info(sprintf("SummitService::copyClonedMediaUploadFiles target %s gone or has no presentation, nothing to copy", $target_media_upload_id)); + return; + } + // UnacquiredLockException propagates: the processor is writing this location, so retry later + $token = $this->lock_service->acquireLock($lock_name, ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME); + self::$held_media_upload_lock_tokens[$lock_name] = $token; + try { + $legs = $this->resolveClonedMediaUploadLegs($source_media_upload_id, $target_media_upload_id, $filename); + $enabled = 0; + $copied = 0; + foreach ($legs as $leg) { + $result = $this->copyClonedMediaUploadLeg($leg['kind'], $leg['storage'], $leg['from'], $leg['to'], $leg['options']); + if (is_null($result)) continue; + $enabled++; + if ($result) $copied++; + } + // a source missing on every enabled leg is a broken original: fail so the retries, + // then failed(), resolve it + if ($enabled > 0 && $copied === 0) + throw new \RuntimeException(sprintf("SummitService::copyClonedMediaUploadFiles source media upload %s has no file on any storage", $source_media_upload_id)); + } finally { + unset(self::$held_media_upload_lock_tokens[$lock_name]); + $this->lock_service->releaseLock($lock_name, $token); + } + } + + /** + * @inheritDoc + */ + public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename): void + { + try { + $lock_name = $this->resolveClonedMediaUploadLockName($target_media_upload_id, $filename); + } catch (\Throwable $ex) { + Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload could not remove clone media upload %s", $target_media_upload_id)); + Log::error($ex); + return; + } + if (is_null($lock_name)) { + Log::info(sprintf("SummitService::removeUncopiedClonedMediaUpload target %s gone or has no presentation, nothing to remove", $target_media_upload_id)); + return; + } + + if (array_key_exists($lock_name, self::$held_media_upload_lock_tokens)) { + // A final-attempt timeout calls the job's failed() from the worker's SIGALRM handler in + // the SAME process, with copyClonedMediaUploadFiles suspended mid-copy still holding this + // lock - its finally never runs before the process is killed. The lock is still ours: + // re-acquiring it here would always throw UnacquiredLockException, so clean up directly + // and release the token we already hold instead. + $token = self::$held_media_upload_lock_tokens[$lock_name]; + Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload target %s copy timed out while still holding its own storage lock, removing the clone's upload", $target_media_upload_id)); + try { + $this->removeClonedMediaUploadTarget($target_media_upload_id, $filename); + } catch (\Throwable $ex) { + // the job is already terminal: this error log is the operator's signal that the clone + // still has an upload with no file behind it (fix: delete it or re-upload on the clone) + Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload could not remove clone media upload %s", $target_media_upload_id)); + Log::error($ex); + } finally { + unset(self::$held_media_upload_lock_tokens[$lock_name]); + $this->lock_service->releaseLock($lock_name, $token); + } + return; + } + + try { + $this->lock_service->lock( + $lock_name, + fn () => $this->removeClonedMediaUploadTarget($target_media_upload_id, $filename), + ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME + ); + } catch (UnacquiredLockException $ex) { + // the lock could not be taken: held by another process (e.g. the processor writing a + // re-upload at this location) or the lock store is unreachable. Either way the record + // can't be safely checked and removed, so keep it and leave it to the operator. + Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload could not take the storage lock to remove clone media upload %s; if the clone's file is missing, delete the upload or re-upload it on the clone", $target_media_upload_id)); + } catch (\Throwable $ex) { + // the job is already terminal: this error log is the operator's signal that the clone + // still has an upload with no file behind it (fix: delete it or re-upload on the clone) + Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload could not remove clone media upload %s", $target_media_upload_id)); + Log::error($ex); + } + } + + /** + * The storage lock for the target's location, or null when the target is gone or has no + * presentation. Read before the lock is taken; the guards re-check the target under it. + * @param int $target_media_upload_id + * @param string $filename + * @return string|null + */ + private function resolveClonedMediaUploadLockName(int $target_media_upload_id, string $filename): ?string + { + return $this->tx_service->transaction(function () use ($target_media_upload_id, $filename) { + $target = $this->presentation_media_upload_repository->getById($target_media_upload_id); + if (!$target instanceof PresentationMediaUpload) return null; + $presentation = $target->getPresentation(); + if (is_null($presentation)) return null; + return self::mediaUploadStorageLockName($presentation->getId(), $filename); + }); + } + + /** + * Resolved inside a short read transaction, under the lock; the copy runs outside the transaction. + * @param int $source_media_upload_id + * @param int $target_media_upload_id + * @param string $filename + * @return array + */ + private function resolveClonedMediaUploadLegs(int $source_media_upload_id, int $target_media_upload_id, string $filename): array + { + return $this->tx_service->transaction(function () use ($source_media_upload_id, $target_media_upload_id, $filename) { + $target = $this->presentation_media_upload_repository->getById($target_media_upload_id); + if (!$target instanceof PresentationMediaUpload) { + Log::info(sprintf("SummitService::resolveClonedMediaUploadLegs target %s gone, nothing to copy", $target_media_upload_id)); + return []; + } + if ($this->isClonedMediaUploadReplaced($target, $filename)) { + Log::info(sprintf("SummitService::resolveClonedMediaUploadLegs target %s was re-uploaded, nothing to copy", $target_media_upload_id)); + return []; + } + $source = $this->presentation_media_upload_repository->getById($source_media_upload_id); + // the clone still needs this file; fail so the retries, then failed(), resolve it + if (!$source instanceof PresentationMediaUpload) + throw new \RuntimeException(sprintf("SummitService::resolveClonedMediaUploadLegs source media upload %s is gone", $source_media_upload_id)); + // a same-filename re-upload of the original leaves the old file in place until the + // processor writes the new one; wait for it rather than copy stale bytes + if ($this->pending_media_upload_repository->hasInFlightForMediaUpload($source->getId())) + throw new \RuntimeException(sprintf("SummitService::resolveClonedMediaUploadLegs source media upload %s has a re-upload in flight", $source_media_upload_id)); + + $type = $target->getMediaUploadType(); + if (is_null($type)) return []; + + return [ + [ + 'kind' => IStorageTypesConstants::PrivateType, + 'storage' => $type->getPrivateStorageType(), + 'from' => $source->getRelativePath(IStorageTypesConstants::PrivateType), + 'to' => $target->getRelativePath(IStorageTypesConstants::PrivateType), + 'options' => [], + ], + [ + 'kind' => IStorageTypesConstants::PublicType, + 'storage' => $type->getPublicStorageType(), + 'from' => $source->getRelativePath(IStorageTypesConstants::PublicType), + 'to' => $target->getRelativePath(IStorageTypesConstants::PublicType), + 'options' => $type->isUseTemporaryLinksOnPublicStorage() ? [] : 'public', + ], + ]; + }); + } + + /** + * A clone that staff have re-uploaded since is never written to or deleted. updateMediaUploadFrom + * keeps the record's ID and may keep its filename, so the signal is the PendingMediaUpload row + * every re-upload creates (a clone never has one). + * @param PresentationMediaUpload $target + * @param string $filename the clone's filename when the copy was queued + * @return bool + */ + private function isClonedMediaUploadReplaced(PresentationMediaUpload $target, string $filename): bool + { + return $target->getFilename() !== $filename + || $this->pending_media_upload_repository->hasAnyForMediaUpload($target->getId()); + } + + /** + * @param string $kind + * @param string|null $storage + * @param string $from + * @param string $to + * @param array|string $options + * @return bool|null null when the leg's storage is disabled, false when its source file is + * missing (skipped), true when copied + */ + private function copyClonedMediaUploadLeg(string $kind, ?string $storage, string $from, string $to, $options): ?bool + { + if (empty($storage)) return null; + $download = FileDownloadStrategyFactory::build($storage); + $upload = FileUploadStrategyFactory::build($storage); + if (is_null($download) || is_null($upload)) return null; // None + + // An original can lack one leg (storage types edited after upload; the public back-fill + // only covers display_on_site uploads). Copy the legs it has rather than fail the clone. + if (!$download->exists($from)) { + Log::warning(sprintf("SummitService::copyClonedMediaUploadLeg %s leg source missing on %s, skipping it: %s", $kind, $storage, $from)); + return false; + } + + // Always overwrite: a killed attempt can leave a truncated file at $to (the Local adapter + // writes straight to the final path), so existence never proves a leg finished. + $stream = $download->readStream($from); + if (!is_resource($stream)) + throw new \RuntimeException(sprintf("SummitService::copyClonedMediaUploadLeg could not read on %s: %s", $storage, $from)); + + try { + if (!$upload->saveFromStream($stream, $to, $options)) + throw new \RuntimeException(sprintf("SummitService::copyClonedMediaUploadLeg could not write on %s: %s", $storage, $to)); + } finally { + if (is_resource($stream)) fclose($stream); + } + Log::info(sprintf("SummitService::copyClonedMediaUploadLeg %s copied %s -> %s", $storage, $from, $to)); + return true; + } + + /** + * @param int $target_media_upload_id + * @param string $filename + */ + private function removeClonedMediaUploadTarget(int $target_media_upload_id, string $filename): void + { + // remove the record first and capture its target-only paths; storage is touched only + // after commit, so a failed commit can never leave a record whose file was moved + $cleanup = $this->tx_service->transaction(function () use ($target_media_upload_id, $filename) { + // FOR UPDATE: a concurrent re-upload's PendingMediaUpload insert waits for this delete + // (or wins, and the guard below sees it) instead of being cascaded away with the record + $target = $this->presentation_media_upload_repository->getByIdExclusiveLock($target_media_upload_id); + if (!$target instanceof PresentationMediaUpload) return []; + if ($this->isClonedMediaUploadReplaced($target, $filename)) { + Log::info(sprintf("SummitService::removeClonedMediaUploadTarget target %s was re-uploaded, keeping it", $target_media_upload_id)); + return []; + } + + $paths = []; + $type = $target->getMediaUploadType(); + if (!is_null($type)) { + foreach ([ + IStorageTypesConstants::PrivateType => $type->getPrivateStorageType(), + IStorageTypesConstants::PublicType => $type->getPublicStorageType(), + ] as $kind => $storage) { + if (!empty($storage)) + $paths[] = [$storage, $target->getPath($kind), $target->getFilename()]; + } + } + + $presentation = $target->getPresentation(); + if (!is_null($presentation)) $presentation->removeMediaUpload($target); + Log::warning(sprintf("SummitService::removeClonedMediaUploadTarget removed clone media upload %s", $target_media_upload_id)); + return $paths; + }); + + // same rename-to-DELETED_ cleanup as PresentationService::deleteMediaUpload + foreach ($cleanup as [$storage, $path, $stored_filename]) { + try { + $strategy = FileUploadStrategyFactory::build($storage); + if (!is_null($strategy)) $strategy->markAsDeleted($path, $stored_filename); + } catch (\Throwable $ex) { + Log::error(sprintf("SummitService::removeClonedMediaUploadTarget could not mark %s/%s deleted on %s", $path, $stored_filename, $storage)); + Log::error($ex); + } + } + } } diff --git a/database/migrations/model/Version20261014120000.php b/database/migrations/model/Version20261014120000.php new file mode 100644 index 000000000..4a994ef88 --- /dev/null +++ b/database/migrations/model/Version20261014120000.php @@ -0,0 +1,55 @@ +hasTable("PresentationMediaUpload") && !$builder->hasColumn("PresentationMediaUpload","FrozenPathSegment") ) { + $builder->table('PresentationMediaUpload', function (Table $table) { + $table->string('FrozenPathSegment', 1024)->setNotnull(false)->setDefault(null); + }); + } + } + + /** + * @param Schema $schema + */ + public function down(Schema $schema):void + { + $builder = new Builder($schema); + if($schema->hasTable("PresentationMediaUpload") && $builder->hasColumn("PresentationMediaUpload","FrozenPathSegment") ) { + $builder->table('PresentationMediaUpload', function (Table $table) { + $table->dropColumn('FrozenPathSegment'); + }); + } + } +} diff --git a/database/migrations/model/Version20261014120001.php b/database/migrations/model/Version20261014120001.php new file mode 100644 index 000000000..18266f33b --- /dev/null +++ b/database/migrations/model/Version20261014120001.php @@ -0,0 +1,61 @@ +connection->fetchAllAssociative(<<addSql('UPDATE PresentationMediaUpload SET FrozenPathSegment = ? WHERE ID = ?', [$segment, intval($row['ID'])]); + } + } + + public function down(Schema $schema): void + { + $this->addSql('UPDATE PresentationMediaUpload SET FrozenPathSegment = NULL WHERE LegacyPathFormat = 0'); + } +} diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php new file mode 100644 index 000000000..d0cd6f31d --- /dev/null +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -0,0 +1,493 @@ +setName('PPTX_' . rand(1, 100000)); + $file_type->setDescription('PPTX'); + $file_type->setAllowedExtensions('PPTX'); + self::$em->persist($file_type); + + self::$type = new SummitMediaUploadType(); + self::$type->setType($file_type); + self::$type->setName('Deck'); + self::$type->setDescription('Deck'); + self::$type->setMaxSize(2048); + self::$type->setMinUploadsQty(0); + self::$type->setMaxUploadsQty(1); + self::$type->setPrivateStorageType(IStorageTypesConstants::Local); + self::$type->setPublicStorageType(IStorageTypesConstants::Local); + self::$summit->addMediaUploadType(self::$type); + + $event_type = self::$summit->getEventTypes()[0]; + self::$original = new Presentation(); + self::$original->setTitle('Original'); + self::$original->setType($event_type); + self::$summit->addEvent(self::$original); + self::$clonePresentation = new Presentation(); + self::$clonePresentation->setTitle('Original'); + self::$clonePresentation->setType($event_type); + self::$summit->addEvent(self::$clonePresentation); + + self::$em->persist(self::$summit); + self::$em->flush(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + parent::tearDown(); + } + + /** @return PresentationMediaUpload[] [$source, $target] */ + private function makePair(bool $modernSource = false): array + { + $source = new PresentationMediaUpload(); + $source->setName('deck'); + $source->setFilename('deck.pptx'); + $source->setMediaUploadType(self::$type); + self::$original->addMediaUpload($source); + if ($modernSource) { + $r = new \ReflectionProperty(PresentationMediaUpload::class, 'legacy_path_format'); + $r->setAccessible(true); + $r->setValue($source, false); + $r = new \ReflectionProperty(PresentationMediaUpload::class, 'frozen_path_segment'); + $r->setAccessible(true); + $r->setValue($source, '1-frozen/2-frozen-title'); + } + $target = $source->clone(); + self::$clonePresentation->addMediaUpload($target); + self::$em->flush(); + return [$source, $target]; + } + + private function seedSource(PresentationMediaUpload $source): void + { + Storage::disk('local')->put($source->getRelativePath(IStorageTypesConstants::PrivateType), 'private-bytes'); + Storage::disk('local')->put($source->getRelativePath(IStorageTypesConstants::PublicType), 'public-bytes'); + } + + /** What updateMediaUploadFrom writes for every re-upload, in the same transaction. */ + private function markReuploaded(PresentationMediaUpload $upload, string $status = PendingMediaUpload::STATUS_PENDING): void + { + $pending = new PendingMediaUpload(); + $pending->setSummitId(self::$summit->getId()); + $pending->setMediaUploadTypeId(self::$type->getId()); + $pending->setMediaUpload($upload); + $pending->setPublicPath($upload->getPath(IStorageTypesConstants::PublicType)); + $pending->setPrivatePath($upload->getPath(IStorageTypesConstants::PrivateType)); + $pending->setFileName($upload->getFilename()); + $pending->setTempFilePath('/tmp/' . $upload->getFilename()); + $pending->setStatus($status); + self::$em->persist($pending); + self::$em->flush(); + } + + private function runJob(CopyClonedMediaUploadFilesJob $job): void + { + $job->handle(App::make(ISummitService::class)); + } + + /** Holds an upload's storage lock, as processPendingMediaUploads does, while $fn runs. */ + private function withStorageLockHeld(PresentationMediaUpload $upload, \Closure $fn): void + { + $locks = App::make(ILockManagerService::class); + $name = SummitService::mediaUploadStorageLockName($upload->getPresentation()->getId(), $upload->getFilename()); + $token = $locks->acquireLock($name, 60); + try { + $fn(); + } finally { + $locks->releaseLock($name, $token); + } + } + + private function findUpload(int $id): ?PresentationMediaUpload + { + self::$em->clear(); + return \LaravelDoctrine\ORM\Facades\Registry::getManager(\models\utils\SilverstripeBaseModel::EntityManager) + ->find(PresentationMediaUpload::class, $id); + } + + public function testQueueSettingsStayBelowRetryAfter(): void + { + $job = new CopyClonedMediaUploadFilesJob(1, 2, 'deck.pptx'); + $this->assertSame(5, $job->tries); + $this->assertSame([30, 60, 120, 300], $job->backoff); + $this->assertLessThan(Config::get('queue.connections.redis.retry_after'), $job->timeout); + $this->assertLessThan(Config::get('queue.connections.database.retry_after'), $job->timeout); + } + + public function testHandleCopiesBothLegsAndLeavesOriginal(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $disk = Storage::disk('local'); + $this->assertSame('private-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertSame('public-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PublicType))); + $this->assertSame('private-bytes', $disk->get($source->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertSame('public-bytes', $disk->get($source->getRelativePath(IStorageTypesConstants::PublicType))); + $this->assertNotSame( + $source->getRelativePath(IStorageTypesConstants::PrivateType), + $target->getRelativePath(IStorageTypesConstants::PrivateType) + ); + } + + public function testHandleCopiesFromModernSourceToLegacyClone(): void + { + [$source, $target] = $this->makePair(true); + $this->seedSource($source); + $this->assertStringContainsString('1-frozen/2-frozen-title', $source->getRelativePath(IStorageTypesConstants::PrivateType)); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $expected = sprintf('%s/%s/deck.pptx', self::$summit->getId(), self::$clonePresentation->getId()); + $this->assertStringEndsWith($expected, $target->getRelativePath(IStorageTypesConstants::PrivateType)); + $this->assertSame('private-bytes', Storage::disk('local')->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + } + + public function testHandleOverwritesPartialTargetFromEarlierAttempt(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + Storage::disk('local')->put($target->getRelativePath(IStorageTypesConstants::PrivateType), 'priv'); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $disk = Storage::disk('local'); + $this->assertSame('private-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertSame('public-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PublicType))); + } + + public function testHandleSkipsStorageTypeNone(): void + { + self::$type->setPublicStorageType(IStorageTypesConstants::None); + self::$em->flush(); + [$source, $target] = $this->makePair(); + Storage::disk('local')->put($source->getRelativePath(IStorageTypesConstants::PrivateType), 'private-bytes'); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $this->assertSame('private-bytes', Storage::disk('local')->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertFalse(Storage::disk('local')->exists($target->getRelativePath(IStorageTypesConstants::PublicType))); + } + + public function testHandleThrowsWhenSourceFileMissing(): void + { + [$source, $target] = $this->makePair(); + + $this->expectException(\RuntimeException::class); + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + } + + /** + * An original with only a private leg (storage types edited after upload; the public back-fill + * only covers display_on_site uploads) still gives the clone the leg it has. + */ + public function testHandleCopiesAvailableLegWhenOtherSourceLegMissing(): void + { + [$source, $target] = $this->makePair(); + Storage::disk('local')->put($source->getRelativePath(IStorageTypesConstants::PrivateType), 'private-bytes'); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $disk = Storage::disk('local'); + $this->assertSame('private-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertFalse($disk->exists($target->getRelativePath(IStorageTypesConstants::PublicType))); + } + + public function testHandleThrowsWhenSourceRecordGone(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $source_id = $source->getId(); + self::$original->removeMediaUpload($source); + self::$em->flush(); + + $this->expectException(\RuntimeException::class); + $this->runJob(new CopyClonedMediaUploadFilesJob($source_id, $target->getId(), 'deck.pptx')); + } + + public function testHandleIsNoOpWhenTargetFilenameChanged(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $target->setFilename('replaced.pptx'); + self::$em->flush(); + Storage::disk('local')->put($target->getRelativePath(IStorageTypesConstants::PrivateType), 'staff-bytes'); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $disk = Storage::disk('local'); + $this->assertSame('staff-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertFalse($disk->exists($target->getRelativePath(IStorageTypesConstants::PublicType))); + } + + public function testHandleIsNoOpWhenTargetReuploadedWithSameFilename(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $this->markReuploaded($target); + Storage::disk('local')->put($target->getRelativePath(IStorageTypesConstants::PrivateType), 'staff-bytes'); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $disk = Storage::disk('local'); + $this->assertSame('staff-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertFalse($disk->exists($target->getRelativePath(IStorageTypesConstants::PublicType))); + } + + public function testHandleIsNoOpWhenTargetGone(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $target_id = $target->getId(); + self::$clonePresentation->removeMediaUpload($target); + self::$em->flush(); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx')); + + $this->assertNull($this->findUpload($target_id)); + } + + public function testFailedRemovesCloneUploadAndKeepsOriginal(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $source_private = $source->getRelativePath(IStorageTypesConstants::PrivateType); + $target_private = $target->getRelativePath(IStorageTypesConstants::PrivateType); + Storage::disk('local')->put($target_private, 'partial'); + $source_id = $source->getId(); + $target_id = $target->getId(); + + (new CopyClonedMediaUploadFilesJob($source_id, $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + + $this->assertNull($this->findUpload($target_id)); + $this->assertNotNull($this->findUpload($source_id)); + $disk = Storage::disk('local'); + $this->assertSame('private-bytes', $disk->get($source_private)); + $this->assertFalse($disk->exists($target_private)); // renamed to DELETED_deck.pptx + } + + /** + * The removal loads the clone FOR UPDATE, so a concurrent re-upload's PendingMediaUpload insert + * serializes against the delete instead of being cascaded away with it. + */ + public function testFailedLoadsTargetUnderExclusiveLock(): void + { + [$source, $target] = $this->makePair(); + $target_id = $target->getId(); + + $real = App::make(IPresentationMediaUploadRepository::class); + $repository = \Mockery::mock(IPresentationMediaUploadRepository::class); + $repository->shouldReceive('getById')->andReturnUsing(fn ($id) => $real->getById($id)); + $repository->shouldReceive('getByIdExclusiveLock')->once()->with($target_id) + ->andReturnUsing(fn ($id) => $real->getByIdExclusiveLock($id)); + App::instance(IPresentationMediaUploadRepository::class, $repository); + // the service singleton takes its repositories at construction; drop any instance already + // resolved so failed() builds one with the mock + App::forgetInstance(ISummitService::class); + + (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + + $this->assertNull($this->findUpload($target_id)); + } + + public function testFailedDoesNotDeleteReplacedUpload(): void + { + [$source, $target] = $this->makePair(); + $target->setFilename('replaced.pptx'); + self::$em->flush(); + $target_id = $target->getId(); + + (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + + $this->assertNotNull($this->findUpload($target_id)); + } + + public function testFailedDoesNotDeleteSameFilenameReupload(): void + { + [$source, $target] = $this->makePair(); + $this->markReuploaded($target); + $target_private = $target->getRelativePath(IStorageTypesConstants::PrivateType); + Storage::disk('local')->put($target_private, 'staff-bytes'); + $target_id = $target->getId(); + + (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + + $this->assertNotNull($this->findUpload($target_id)); + $this->assertSame('staff-bytes', Storage::disk('local')->get($target_private)); + } + + public function testHandleThrowsWhenTargetStorageLocked(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + + $this->withStorageLockHeld($target, function () use ($source, $target) { + try { + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->fail('expected UnacquiredLockException'); + } catch (UnacquiredLockException $ex) { + $this->assertFalse(Storage::disk('local')->exists($target->getRelativePath(IStorageTypesConstants::PrivateType))); + } + }); + } + + /** + * summit-admin re-uploads by delete-then-add, so the processor's row belongs to a NEW upload + * record at the same storage path. The lock is keyed on that location, so the copy still waits. + */ + public function testHandleThrowsWhenSameLocationLockedByDifferentUpload(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + + $locks = App::make(ILockManagerService::class); + // the name a processor writing a DIFFERENT upload record at the clone's deck.pptx would hold + $name = SummitService::mediaUploadStorageLockName(self::$clonePresentation->getId(), 'deck.pptx'); + $token = $locks->acquireLock($name, 60); + try { + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->fail('expected UnacquiredLockException'); + } catch (UnacquiredLockException $ex) { + $disk = Storage::disk('local'); + $this->assertFalse($disk->exists($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertFalse($disk->exists($target->getRelativePath(IStorageTypesConstants::PublicType))); + } finally { + $locks->releaseLock($name, $token); + } + } + + public function testFailedKeepsUploadWhenTargetStorageLocked(): void + { + [$source, $target] = $this->makePair(); + $target_id = $target->getId(); + + Log::spy(); + $this->withStorageLockHeld($target, function () use ($source, $target_id) { + (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + }); + + $this->assertNotNull($this->findUpload($target_id)); + // a lock miss can also be Redis being down, so the log names no cause, only the operator action + Log::shouldHaveReceived('error')->withArgs(fn ($message) => is_string($message) + && str_contains($message, sprintf('could not take the storage lock to remove clone media upload %s', $target_id)) + && str_contains($message, 'delete the upload or re-upload it on the clone')); + } + + /** + * Simulates a final-attempt timeout: Laravel's Worker::registerTimeoutHandler calls failed() + * from the SIGALRM handler in the SAME process while handle() is suspended mid-copy still + * holding the target's storage lock (its finally never runs before the process is killed). + */ + public function testFailedAfterTimeoutRemovesCloneUploadUnderItsOwnLock(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $source_private = $source->getRelativePath(IStorageTypesConstants::PrivateType); + $target_private = $target->getRelativePath(IStorageTypesConstants::PrivateType); + Storage::disk('local')->put($target_private, 'partial'); + $source_id = $source->getId(); + $target_id = $target->getId(); + + $lock_name = SummitService::mediaUploadStorageLockName(self::$clonePresentation->getId(), 'deck.pptx'); + $locks = App::make(ILockManagerService::class); + $token = $locks->acquireLock($lock_name, 60); + + $held = new \ReflectionProperty(SummitService::class, 'held_media_upload_lock_tokens'); + $held->setAccessible(true); + $held->setValue(null, [$lock_name => $token]); + + try { + (new CopyClonedMediaUploadFilesJob($source_id, $target_id, 'deck.pptx')) + ->failed(new \Illuminate\Queue\TimeoutExceededException('timed out')); + } finally { + $held->setValue(null, []); + } + + $this->assertNull($this->findUpload($target_id)); + $this->assertNotNull($this->findUpload($source_id)); + $disk = Storage::disk('local'); + $this->assertSame('private-bytes', $disk->get($source_private)); + $this->assertFalse($disk->exists($target_private)); // renamed to DELETED_deck.pptx + + // the lock must have been released by failed(), not left held: a fresh acquire succeeds + $fresh_token = $locks->acquireLock($lock_name, 60); + $locks->releaseLock($lock_name, $fresh_token); + } + + public function testHandleThrowsWhenSourceReuploadInFlight(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $this->markReuploaded($source, PendingMediaUpload::STATUS_PROCESSING); + + try { + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->fail('expected RuntimeException'); + } catch (\RuntimeException $ex) { + $this->assertStringContainsString('in flight', $ex->getMessage()); + $this->assertFalse(Storage::disk('local')->exists($target->getRelativePath(IStorageTypesConstants::PrivateType))); + } + } + + public function testHandleCopiesWhenSourceReuploadCompleted(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $this->markReuploaded($source, PendingMediaUpload::STATUS_COMPLETED); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $this->assertSame('private-bytes', Storage::disk('local')->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + } +} diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index f2c2fd1b3..75bd5900c 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -20,6 +20,19 @@ use models\summit\SummitEventType; use App\Models\Foundation\Summit\Events\SummitEventTypeConstants; use services\model\ISummitService; +use App\Jobs\CopyClonedMediaUploadFilesJob; +use App\Models\Utils\IStorageTypesConstants; +use App\Services\Utils\ILockManagerService; +use Illuminate\Support\Facades\Bus; +use Illuminate\Support\Facades\Config; +use LaravelDoctrine\ORM\Facades\Registry; +use models\summit\PendingMediaUpload; +use models\summit\Presentation; +use models\summit\PresentationMediaUpload; +use models\summit\SummitMediaFileType; +use models\summit\SummitMediaUploadType; +use models\utils\SilverstripeBaseModel; +use services\model\SummitService; /** * Class SummitServiceTest @@ -342,4 +355,199 @@ public function testProcessEventDataImportsOnlyValidRowsWhenMostRowsFail() 'Known per-row failures must not block the source file deletion' ); } + + public function testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsLock(): void + { + $presentation = self::$summit->getPresentations()[0]; + $upload = $presentation->getMediaUploads()->first(); + + $pending = new PendingMediaUpload(); + $pending->setSummitId(self::$summit->getId()); + $pending->setMediaUploadTypeId($upload->getMediaUploadType()->getId()); + $pending->setMediaUpload($upload); + $pending->setPublicPath($upload->getPath(IStorageTypesConstants::PublicType)); + $pending->setPrivatePath($upload->getPath(IStorageTypesConstants::PrivateType)); + $pending->setFileName($upload->getFilename()); + $pending->setTempFilePath('upload/does-not-exist/' . $upload->getFilename()); + $pending->setStatus(PendingMediaUpload::STATUS_PENDING); + self::$em->persist($pending); + self::$em->flush(); + $pending_id = $pending->getId(); + + $reload = function () use ($pending_id): PendingMediaUpload { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + return $em->find(PendingMediaUpload::class, $pending_id); + }; + + $locks = App::make(ILockManagerService::class); + $name = SummitService::mediaUploadStorageLockName($presentation->getId(), $upload->getFilename()); + $token = $locks->acquireLock($name, 60); + try { + App::make(ISummitService::class)->processPendingMediaUploads(); + } finally { + $locks->releaseLock($name, $token); + } + + $row = $reload(); + $this->assertSame(PendingMediaUpload::STATUS_PENDING, $row->getStatus()); + $this->assertSame(0, $row->getAttempts()); + + // control: with the lock free the same row IS attempted (its temp file does not exist, + // so the attempt fails and is counted), which proves the lock is what skipped it above + App::make(ISummitService::class)->processPendingMediaUploads(); + $this->assertSame(1, $reload()->getAttempts()); + + // the processor must release its lock when done; a missing finally would leave the + // name locked and this acquire would throw + $locks->releaseLock($name, $locks->acquireLock($name, 1)); + } + + /** + * Every InsertSummitTestData presentation already carries one media upload; clear the chosen + * presentation's materials so the test controls exactly which uploads get cloned. + */ + private function presentationWithoutMaterials(): Presentation + { + $presentation = self::$summit->getPresentations()[0]; + // removeMediaUpload, not a bare collection removeElement: it also unsets the owning + // side (PresentationID), so the removal is flushed and cloneEvent's reload cannot see it + foreach ($presentation->getMediaUploads()->toArray() as $media_upload) + $presentation->removeMediaUpload($media_upload); + $this->assertCount(0, $presentation->getMaterials()->toArray(), 'fixture presentation has non-upload materials; remove them too'); + self::$em->flush(); + return $presentation; + } + + private function presentationWithModernMediaUpload(): array + { + $file_type = new SummitMediaFileType(); + $file_type->setName('PPTX_' . rand(1, 100000)); + $file_type->setDescription('PPTX'); + $file_type->setAllowedExtensions('PPTX'); + self::$em->persist($file_type); + + $type = new SummitMediaUploadType(); + $type->setType($file_type); + $type->setName('Deck'); + $type->setDescription('Deck'); + $type->setMaxSize(2048); + $type->setMinUploadsQty(0); + $type->setMaxUploadsQty(1); + $type->setPrivateStorageType(IStorageTypesConstants::Local); + $type->setPublicStorageType(IStorageTypesConstants::Local); + self::$summit->addMediaUploadType($type); + + $presentation = $this->presentationWithoutMaterials(); + $upload = new PresentationMediaUpload(); + $upload->setName('deck'); + $upload->setFilename('deck.pptx'); + $upload->setMediaUploadType($type); + $presentation->addMediaUpload($upload); + // modern format, so the test proves the clone resets to legacy (ticket acceptance) + $r = new \ReflectionProperty(PresentationMediaUpload::class, 'legacy_path_format'); + $r->setAccessible(true); + $r->setValue($upload, false); + $r = new \ReflectionProperty(PresentationMediaUpload::class, 'frozen_path_segment'); + $r->setAccessible(true); + $r->setValue($upload, sprintf('%s-x/%s-y', self::$summit->getId(), $presentation->getId())); + + self::$em->persist(self::$summit); + self::$em->flush(); + + Storage::disk('local')->put($upload->getRelativePath(IStorageTypesConstants::PrivateType), 'private-bytes'); + Storage::disk('local')->put($upload->getRelativePath(IStorageTypesConstants::PublicType), 'public-bytes'); + return [$presentation, $upload]; + } + + public function testCloneEventQueuesCopyAndCopyLandsFiles(): void + { + Storage::fake('local'); + Bus::fake(); + [$presentation, $upload] = $this->presentationWithModernMediaUpload(); + + $clone = App::make(ISummitService::class)->cloneEvent(self::$summit, $presentation->getId()); + + $this->assertInstanceOf(Presentation::class, $clone); + $clone_uploads = array_values($clone->getMediaUploads()->toArray()); + $this->assertCount(1, $clone_uploads); + $clone_upload = $clone_uploads[0]; + $this->assertSame('deck.pptx', $clone_upload->getFilename()); + $this->assertStringEndsWith( + sprintf('/%s/%s/deck.pptx', self::$summit->getId(), $clone->getId()), + $clone_upload->getRelativePath(IStorageTypesConstants::PrivateType) + ); + + Bus::assertDispatchedTimes(CopyClonedMediaUploadFilesJob::class, 1); + Bus::assertDispatched(CopyClonedMediaUploadFilesJob::class, function (CopyClonedMediaUploadFilesJob $job) use ($upload, $clone_upload) { + return $job->source_media_upload_id === $upload->getId() + && $job->target_media_upload_id === $clone_upload->getId() + && $job->filename === 'deck.pptx'; + }); + + $job = Bus::dispatched(CopyClonedMediaUploadFilesJob::class)->first(); + $job->handle(App::make(ISummitService::class)); + + $disk = Storage::disk('local'); + $this->assertSame('private-bytes', $disk->get($clone_upload->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertSame('public-bytes', $disk->get($clone_upload->getRelativePath(IStorageTypesConstants::PublicType))); + $this->assertSame('private-bytes', $disk->get($upload->getRelativePath(IStorageTypesConstants::PrivateType))); + } + + /** + * Both queues down AND the synchronous fallback failing: the clone is already committed, so + * cloneEvent still returns it rather than surfacing the copy failure to the caller. + */ + public function testCloneEventStillReturnsCloneWhenCopyDispatchFails(): void + { + Storage::fake('local'); + [$presentation, $upload] = $this->presentationWithModernMediaUpload(); + + // every path JobDispatcher::withDbFallback takes (primary, database, sync) fails for the copy + // job only; other jobs cloneEvent dispatches are swallowed as Bus::fake would + $fail_copy = function ($job) { + if ($job instanceof CopyClonedMediaUploadFilesJob) throw new \RuntimeException('queue down'); + return null; + }; + Bus::shouldReceive('dispatch')->andReturnUsing($fail_copy); + Bus::shouldReceive('dispatchSync')->atLeast()->once()->andReturnUsing($fail_copy); + + $clone = App::make(ISummitService::class)->cloneEvent(self::$summit, $presentation->getId()); + + $this->assertInstanceOf(Presentation::class, $clone); + $this->assertGreaterThan(0, $clone->getId()); + $this->assertNotSame($presentation->getId(), $clone->getId()); + + // By the time cloneEvent returns, the cleanup call in its catch has already run + // synchronously and removed the clone's upload from the in-memory collection too - so the + // path has to be derived rather than read off the (already gone) clone upload entity. The + // cloned upload is created via PresentationMediaUpload::createInstance(), which always starts + // legacy_path_format=true, so its path is the legacy {mount}/Private/{summit_id}/{clone_id}/{filename}. + $clone_id = $clone->getId(); + $clone_private_path = sprintf( + '%s/%s/%s/%s/%s', + Config::get('mediaupload.private_mounting_folder'), + IStorageTypesConstants::PrivateType, + self::$summit->getId(), + $clone_id, + $upload->getFilename() + ); + + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + /** @var Presentation $reloaded_clone */ + $reloaded_clone = $em->find(Presentation::class, $clone_id); + $this->assertCount(0, $reloaded_clone->getMediaUploads()); + $this->assertFalse(Storage::disk('local')->exists($clone_private_path)); + } + + public function testCloneEventWithoutMediaUploadsQueuesNothing(): void + { + Bus::fake(); + $presentation = $this->presentationWithoutMaterials(); + + App::make(ISummitService::class)->cloneEvent(self::$summit, $presentation->getId()); + + Bus::assertNotDispatched(CopyClonedMediaUploadFilesJob::class); + } } diff --git a/tests/Unit/Entities/PresentationMediaUploadPathTest.php b/tests/Unit/Entities/PresentationMediaUploadPathTest.php new file mode 100644 index 000000000..f6c15a6a2 --- /dev/null +++ b/tests/Unit/Entities/PresentationMediaUploadPathTest.php @@ -0,0 +1,127 @@ +setFilename('deck.pptx'); + $presentation->addMediaUpload($upload); + $r = new \ReflectionProperty(PresentationMediaUpload::class, 'legacy_path_format'); + $r->setAccessible(true); + $r->setValue($upload, $legacy); + $r = new \ReflectionProperty(PresentationMediaUpload::class, 'frozen_path_segment'); + $r->setAccessible(true); + $r->setValue($upload, $frozen); + return $upload; + } + + private function presentation(): Presentation + { + $p = self::$summit->getPresentations()[0]; + $p->setTitle('Original Title: Café & Co'); + return $p; + } + + public function testSlugifyMatchesGenerateSlug(): void + { + $p = $this->presentation(); + $p->generateSlug(); + $this->assertSame($p->getSlug(), Presentation::slugify('Original Title: Café & Co')); + } + + public function testLegacyPathUnchanged(): void + { + $p = $this->presentation(); + $u = $this->makeUpload($p, true); + $s = self::$summit->getId(); + $this->assertSame(sprintf('PresentationMediaUploads/%s/%s', $s, $p->getId()), $u->getPath(IStorageTypesConstants::PublicType)); + $this->assertSame(sprintf('PresentationMediaUploads/Private/%s/%s', $s, $p->getId()), $u->getPath(IStorageTypesConstants::PrivateType)); + } + + public function testModernPathWithoutFrozenSegmentMatchesOldFormula(): void + { + $p = $this->presentation(); + $u = $this->makeUpload($p, false); + $summit = self::$summit; + $expected = sprintf( + 'PresentationMediaUploads/%s-%s/%s-%s', + $summit->getId(), filter_var($summit->getRawSlug(), FILTER_SANITIZE_ENCODED), + $p->getId(), filter_var(Presentation::slugify($p->getTitle()), FILTER_SANITIZE_ENCODED) + ); + $this->assertSame($expected, $u->getPath(IStorageTypesConstants::PublicType)); + } + + public function testBuildModernPathSegmentMatchesUnfrozenPath(): void + { + $p = $this->presentation(); + $u = $this->makeUpload($p, false); + $segment = PresentationMediaUpload::buildModernPathSegment( + self::$summit->getId(), self::$summit->getRawSlug(), $p->getId(), Presentation::slugify($p->getTitle()) + ); + $this->assertSame('PresentationMediaUploads/' . $segment, $u->getPath(IStorageTypesConstants::PublicType)); + $this->assertSame('PresentationMediaUploads/Private/' . $segment, $u->getPath(IStorageTypesConstants::PrivateType)); + } + + public function testFrozenSegmentSurvivesTitleChange(): void + { + $p = $this->presentation(); + $u = $this->makeUpload($p, false, '7-frozen-summit/99-frozen-title'); + $before = $u->getPath(IStorageTypesConstants::PrivateType); + $p->setTitle('A Completely Different Title'); + $this->assertSame($before, $u->getPath(IStorageTypesConstants::PrivateType)); + $this->assertSame('PresentationMediaUploads/Private/7-frozen-summit/99-frozen-title', $before); + } + + public function testFrozenSegmentSurvivesSummitSlugChange(): void + { + $p = $this->presentation(); + $u = $this->makeUpload($p, false, '7-frozen-summit/99-frozen-title'); + $before = $u->getPath(IStorageTypesConstants::PublicType); + self::$summit->setRawSlug('renamed summit'); + $this->assertSame($before, $u->getPath(IStorageTypesConstants::PublicType)); + } + + public function testCloneDoesNotCarryFrozenSegment(): void + { + $p = $this->presentation(); + $u = $this->makeUpload($p, false, '7-frozen-summit/99-frozen-title'); + $clone = $u->clone(); + $this->assertNull($clone->getFrozenPathSegment()); + } +} diff --git a/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php b/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php new file mode 100644 index 000000000..0c1c72af3 --- /dev/null +++ b/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php @@ -0,0 +1,87 @@ +newInstanceWithoutConstructor(); + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('fetchAllAssociative')->once()->andReturn($rows); + $p = new \ReflectionProperty(\Doctrine\Migrations\AbstractMigration::class, 'connection'); + $p->setAccessible(true); + $p->setValue($migration, $conn); + return $migration; + } + + public function testUsesTitleSlugWhenTitlePresent(): void + { + $m = $this->migrationWithRows([[ + 'ID' => 5, 'SummitID' => 12, 'SummitSlug' => 'summit-2020', + 'PresentationID' => 300, 'Title' => 'My Talk: Part 1', 'PresentationSlug' => 'stale-slug', + ]]); + $m->up(new Schema()); + $sql = $m->getSql(); + $this->assertCount(1, $sql); + $this->assertStringContainsString('UPDATE PresentationMediaUpload SET FrozenPathSegment = ?', $sql[0]->getStatement()); + $this->assertSame( + ['12-summit-2020/300-' . filter_var(Presentation::slugify('My Talk: Part 1'), FILTER_SANITIZE_ENCODED), 5], + $sql[0]->getParameters() + ); + } + + public function testFallsBackToStoredSlugWhenTitleEmpty(): void + { + $m = $this->migrationWithRows([[ + 'ID' => 6, 'SummitID' => 8, 'SummitSlug' => 's8', + 'PresentationID' => 301, 'Title' => '', 'PresentationSlug' => 'kept-slug', + ]]); + $m->up(new Schema()); + $sql = $m->getSql(); + $this->assertSame(['8-s8/301-kept-slug', 6], $sql[0]->getParameters()); + } + + public function testUsesStoredSlugWhenTitleIsZero(): void + { + $m = $this->migrationWithRows([[ + 'ID' => 7, 'SummitID' => 9, 'SummitSlug' => 's9', + 'PresentationID' => 302, 'Title' => '0', 'PresentationSlug' => 'kept-slug', + ]]); + $m->up(new Schema()); + $sql = $m->getSql(); + $this->assertSame(['9-s9/302-kept-slug', 7], $sql[0]->getParameters()); + } + + public function testNoRowsNoSql(): void + { + $m = $this->migrationWithRows([]); + $m->up(new Schema()); + $this->assertSame([], $m->getSql()); + } +} diff --git a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php index 436bf97e2..f48accc5c 100644 --- a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php +++ b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php @@ -13,13 +13,18 @@ **/ use App\Models\Foundation\Summit\Repositories\IPendingMediaUploadRepository; +use App\Services\Utils\Exceptions\UnacquiredLockException; +use App\Services\Utils\ILockManagerService; use Illuminate\Container\Container; use Illuminate\Support\Facades\Facade; +use services\model\ISummitService; use services\model\SummitService; use libs\utils\ITransactionService; use Mockery; use models\summit\ISummitRepository; use models\summit\PendingMediaUpload; +use models\summit\Presentation; +use models\summit\PresentationMediaUpload; use models\summit\Summit; use models\summit\SummitMediaUploadType; use PHPUnit\Framework\TestCase; @@ -71,7 +76,8 @@ protected function tearDown(): void private function createServiceWithDeps( IPendingMediaUploadRepository $pendingRepo, ITransactionService $txService, - ?ISummitRepository $summitRepository = null + ?ISummitRepository $summitRepository = null, + ?ILockManagerService $lockService = null ): SummitService { $ref = new \ReflectionClass(SummitService::class); $service = $ref->newInstanceWithoutConstructor(); @@ -86,6 +92,17 @@ private function createServiceWithDeps( $prop->setValue($service, $summitRepository); } + // Default: a lock that is always free, so tests that don't care about locking + // exercise only the per-row processing. + if (!$lockService) { + $lockService = Mockery::mock(ILockManagerService::class); + $lockService->shouldReceive('acquireLock')->andReturn('test-token'); + $lockService->shouldReceive('releaseLock')->andReturnNull(); + } + $prop = $ref->getProperty('lock_service'); + $prop->setAccessible(true); + $prop->setValue($service, $lockService); + // tx_service is protected in AbstractService (grandparent of SummitService) $parentRef = $ref; while ($parentRef && !$parentRef->hasProperty('tx_service')) { @@ -100,6 +117,16 @@ private function createServiceWithDeps( return $service; } + /** A media upload mock on presentation $presentation_id, as the processor's lock name needs. */ + private function mediaUploadMock(int $presentation_id): PresentationMediaUpload + { + $presentation = Mockery::mock(Presentation::class); + $presentation->shouldReceive('getId')->andReturn($presentation_id); + $mediaUpload = Mockery::mock(PresentationMediaUpload::class); + $mediaUpload->shouldReceive('getPresentation')->andReturn($presentation); + return $mediaUpload; + } + /** * Test max retries exceeded marks upload as Error. * When attempts >= max_retries, the upload should be permanently marked as Error. @@ -142,8 +169,12 @@ public function testTransientFailureKeepsUploadPendingForRetry(): void $txService = Mockery::mock(ITransactionService::class); $summitRepository = Mockery::mock(ISummitRepository::class); + $mediaUpload = $this->mediaUploadMock(101); + $pendingUpload = Mockery::mock(PendingMediaUpload::class); $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($mediaUpload); + $pendingUpload->shouldReceive('getFileName')->andReturn('deck.pptx'); // First getAttempts() call: retry guard check (0 < 3, passes) // Second getAttempts() call: in catch block (1 < 3, no ERROR status) $pendingUpload->shouldReceive('getAttempts')->andReturnValues([0, 1]); @@ -316,8 +347,12 @@ public function testPartialStatusPreservedOnFailure(): void $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); $txService = Mockery::mock(ITransactionService::class); + $mediaUpload = $this->mediaUploadMock(102); + $pendingUpload = Mockery::mock(PendingMediaUpload::class); $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($mediaUpload); + $pendingUpload->shouldReceive('getFileName')->andReturn('deck.pptx'); // First call: retry guard (1 < 3, passes) // Second call: in catch block (2 < 3, leave at partial status) $pendingUpload->shouldReceive('getAttempts')->andReturnValues([1, 2]); @@ -342,4 +377,92 @@ public function testPartialStatusPreservedOnFailure(): void $this->assertEquals(0, $stats['processed']); $this->assertEquals(1, $stats['errors']); } + + /** + * Unit-level counterpart of SummitServiceTest::testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsLock. + * When the upload's storage lock is held (e.g. by copyClonedMediaUploadFiles), the row + * must be left completely untouched: no status/attempts mutation, and since the lock was + * never acquired, no release either. + */ + public function testLockedUploadIsSkippedWithoutTouchingIt(): void + { + $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); + $txService = Mockery::mock(ITransactionService::class); + + $mediaUpload = $this->mediaUploadMock(103); + + $pendingUpload = Mockery::mock(PendingMediaUpload::class); + $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($mediaUpload); + $pendingUpload->shouldReceive('getFileName')->andReturn('deck.pptx'); + $pendingUpload->shouldReceive('getAttempts')->andReturn(0); + $pendingUpload->shouldNotReceive('setStatus'); + $pendingUpload->shouldNotReceive('incrementAttempts'); + $pendingUpload->shouldNotReceive('setErrorMessage'); + + $lockService = Mockery::mock(ILockManagerService::class); + $lockService->shouldReceive('acquireLock')->once() + ->with(SummitService::mediaUploadStorageLockName(103, 'deck.pptx'), ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME) + ->andThrow(new UnacquiredLockException('upload 1 storage is locked')); + $lockService->shouldNotReceive('releaseLock'); + + $pendingRepo->shouldReceive('resetStuckProcessingRows')->once()->with(10)->andReturn(0); + $pendingRepo->shouldReceive('getPendingUploads')->once()->andReturn([$pendingUpload]); + $pendingRepo->shouldReceive('deleteCompletedOlderThan')->once()->with(7, 1000)->andReturn(0); + + // 2 transactions: reset stuck, cleanup — the row's own processing never opens one + $txService->shouldReceive('transaction')->twice()->andReturnUsing(function ($callback) { + return $callback(); + }); + + $service = $this->createServiceWithDeps($pendingRepo, $txService, null, $lockService); + + $stats = $service->processPendingMediaUploads(); + + $this->assertEquals(0, $stats['processed']); + $this->assertEquals(0, $stats['errors']); + } + + /** + * A row whose media upload has no presentation has no storage location to lock (nor a valid + * path to write); it is left untouched rather than aborting the whole run. + */ + public function testUploadWithoutPresentationIsSkippedWithoutTouchingIt(): void + { + $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); + $txService = Mockery::mock(ITransactionService::class); + + $mediaUpload = Mockery::mock(PresentationMediaUpload::class); + $mediaUpload->shouldReceive('getId')->andReturn(104); + $mediaUpload->shouldReceive('getPresentation')->andReturn(null); + + $pendingUpload = Mockery::mock(PendingMediaUpload::class); + $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($mediaUpload); + $pendingUpload->shouldReceive('getFileName')->andReturn('deck.pptx'); + $pendingUpload->shouldReceive('getAttempts')->andReturn(0); + $pendingUpload->shouldNotReceive('setStatus'); + $pendingUpload->shouldNotReceive('incrementAttempts'); + $pendingUpload->shouldNotReceive('setErrorMessage'); + + $lockService = Mockery::mock(ILockManagerService::class); + $lockService->shouldNotReceive('acquireLock'); + $lockService->shouldNotReceive('releaseLock'); + + $pendingRepo->shouldReceive('resetStuckProcessingRows')->once()->with(10)->andReturn(0); + $pendingRepo->shouldReceive('getPendingUploads')->once()->andReturn([$pendingUpload]); + $pendingRepo->shouldReceive('deleteCompletedOlderThan')->once()->with(7, 1000)->andReturn(0); + + // 2 transactions: reset stuck, cleanup + $txService->shouldReceive('transaction')->twice()->andReturnUsing(function ($callback) { + return $callback(); + }); + + $service = $this->createServiceWithDeps($pendingRepo, $txService, null, $lockService); + + $stats = $service->processPendingMediaUploads(); + + $this->assertEquals(0, $stats['processed']); + $this->assertEquals(0, $stats['errors']); + } }