From 84846fc913b6aa101abb54049ab3bb3e11eb1743 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 13:51:01 -0700 Subject: [PATCH 01/13] fix(media-uploads): freeze modern-format upload paths so a rename cannot move them --- .github/workflows/push.yml | 1 + .../Materials/PresentationMediaUpload.php | 34 ++++- .../Events/Presentations/Presentation.php | 14 +- .../model/Version20261014120000.php | 50 +++++++ .../model/Version20261014120001.php | 56 ++++++++ .../PresentationMediaUploadPathTest.php | 127 ++++++++++++++++++ ...FrozenPathSegmentBackfillMigrationTest.php | 76 +++++++++++ 7 files changed, 351 insertions(+), 7 deletions(-) create mode 100644 database/migrations/model/Version20261014120000.php create mode 100644 database/migrations/model/Version20261014120001.php create mode 100644 tests/Unit/Entities/PresentationMediaUploadPathTest.php create mode 100644 tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 2eafddb4be..1616278338 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" } diff --git a/app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMediaUpload.php b/app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMediaUpload.php index 77d509d6aa..319e367763 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 d2e5c9c786..cf1a6ff785 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/database/migrations/model/Version20261014120000.php b/database/migrations/model/Version20261014120000.php new file mode 100644 index 0000000000..1ed14cc2a2 --- /dev/null +++ b/database/migrations/model/Version20261014120000.php @@ -0,0 +1,50 @@ +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 0000000000..750d21c38c --- /dev/null +++ b/database/migrations/model/Version20261014120001.php @@ -0,0 +1,56 @@ +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/Unit/Entities/PresentationMediaUploadPathTest.php b/tests/Unit/Entities/PresentationMediaUploadPathTest.php new file mode 100644 index 0000000000..f6c15a6a20 --- /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 0000000000..12327f7f55 --- /dev/null +++ b/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php @@ -0,0 +1,76 @@ +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 testNoRowsNoSql(): void + { + $m = $this->migrationWithRows([]); + $m->up(new Schema()); + $this->assertSame([], $m->getSql()); + } +} From c99b46e4fb6705bcb88801fe982bef7f06cc68ac Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 14:08:03 -0700 Subject: [PATCH 02/13] fix(media-uploads): add job that copies a cloned media upload's files to the clone's path --- .github/workflows/push.yml | 2 +- app/Jobs/CopyClonedMediaUploadFilesJob.php | 236 +++++++++++ .../IPendingMediaUploadRepository.php | 16 + .../DoctrinePendingMediaUploadRepository.php | 36 ++ tests/CopyClonedMediaUploadFilesJobTest.php | 383 ++++++++++++++++++ 5 files changed, 672 insertions(+), 1 deletion(-) create mode 100644 app/Jobs/CopyClonedMediaUploadFilesJob.php create mode 100644 tests/CopyClonedMediaUploadFilesJobTest.php diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 1616278338..162912ab48 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -79,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 0000000000..65af5a7751 --- /dev/null +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -0,0 +1,236 @@ +source_media_upload_id, $this->target_media_upload_id)); + + // UnacquiredLockException propagates: the processor is writing this upload, so retry later + $lock_service->lock( + self::storageLockName($this->target_media_upload_id), + function () use ($repository, $pending_repository, $tx_service) { + $legs = $this->resolveLegs($repository, $pending_repository, $tx_service); + foreach ($legs as $leg) { + $this->copyLeg($leg['storage'], $leg['from'], $leg['to'], $leg['options']); + } + }, + self::STORAGE_LOCK_LIFETIME + ); + } + + /** + * Resolved inside a short read transaction, under the lock; the copy runs outside the transaction. + */ + private function resolveLegs( + IPresentationMediaUploadRepository $repository, + IPendingMediaUploadRepository $pending_repository, + ITransactionService $tx_service + ): array + { + return $tx_service->transaction(function () use ($repository, $pending_repository) { + $target = $repository->getById($this->target_media_upload_id); + if (!$target instanceof PresentationMediaUpload) { + Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s gone, nothing to copy", $this->target_media_upload_id)); + return []; + } + if ($this->isReplaced($target, $pending_repository)) { + Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s was re-uploaded, nothing to copy", $this->target_media_upload_id)); + return []; + } + $source = $repository->getById($this->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("CopyClonedMediaUploadFilesJob source media upload %s is gone", $this->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 ($pending_repository->hasInFlightForMediaUpload($source->getId())) + throw new \RuntimeException(sprintf("CopyClonedMediaUploadFilesJob source media upload %s has a re-upload in flight", $this->source_media_upload_id)); + + $type = $target->getMediaUploadType(); + if (is_null($type)) return []; + + return [ + [ + 'storage' => $type->getPrivateStorageType(), + 'from' => $source->getRelativePath(IStorageTypesConstants::PrivateType), + 'to' => $target->getRelativePath(IStorageTypesConstants::PrivateType), + 'options' => [], + ], + [ + 'storage' => $type->getPublicStorageType(), + 'from' => $source->getRelativePath(IStorageTypesConstants::PublicType), + 'to' => $target->getRelativePath(IStorageTypesConstants::PublicType), + 'options' => $type->isUseTemporaryLinksOnPublicStorage() ? [] : 'public', + ], + ]; + }); + } + + private function isReplaced(PresentationMediaUpload $target, IPendingMediaUploadRepository $pending_repository): bool + { + return $target->getFilename() !== $this->filename + || $pending_repository->hasAnyForMediaUpload($target->getId()); + } + + private function copyLeg(?string $storage, string $from, string $to, $options): void + { + if (empty($storage)) return; + $download = FileDownloadStrategyFactory::build($storage); + $upload = FileUploadStrategyFactory::build($storage); + if (is_null($download) || is_null($upload)) return; // None + + // 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("CopyClonedMediaUploadFilesJob source missing on %s: %s", $storage, $from)); + + try { + if (!$upload->saveFromStream($stream, $to, $options)) + throw new \RuntimeException(sprintf("CopyClonedMediaUploadFilesJob could not write on %s: %s", $storage, $to)); + } finally { + if (is_resource($stream)) fclose($stream); + } + Log::info(sprintf("CopyClonedMediaUploadFilesJob::copyLeg %s copied %s -> %s", $storage, $from, $to)); + } + + 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() + )); + + try { + App::make(ILockManagerService::class)->lock( + self::storageLockName($this->target_media_upload_id), + fn () => $this->removeTarget(), + self::STORAGE_LOCK_LIFETIME + ); + } catch (UnacquiredLockException $ex) { + // only the processor writing a re-upload of this clone holds the lock, so keep the record + Log::warning(sprintf("CopyClonedMediaUploadFilesJob::failed target %s storage is locked by a re-upload, keeping it", $this->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("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); + Log::error($ex); + } + } + + private function removeTarget(): void + { + $repository = App::make(IPresentationMediaUploadRepository::class); + $pending_repository = App::make(IPendingMediaUploadRepository::class); + + // 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 = App::make(ITransactionService::class)->transaction(function () use ($repository, $pending_repository) { + $target = $repository->getById($this->target_media_upload_id); + if (!$target instanceof PresentationMediaUpload) return []; + if ($this->isReplaced($target, $pending_repository)) { + Log::info(sprintf("CopyClonedMediaUploadFilesJob::failed target %s was re-uploaded, keeping it", $this->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("CopyClonedMediaUploadFilesJob::failed removed clone media upload %s", $this->target_media_upload_id)); + return $paths; + }); + + // same rename-to-DELETED_ cleanup as PresentationService::deleteMediaUpload + foreach ($cleanup as [$storage, $path, $filename]) { + try { + $strategy = FileUploadStrategyFactory::build($storage); + if (!is_null($strategy)) $strategy->markAsDeleted($path, $filename); + } catch (\Throwable $ex) { + Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not mark %s/%s deleted on %s", $path, $filename, $storage)); + Log::error($ex); + } + } + } +} diff --git a/app/Models/Foundation/Summit/Repositories/IPendingMediaUploadRepository.php b/app/Models/Foundation/Summit/Repositories/IPendingMediaUploadRepository.php index 3424227f91..e0221033e4 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 4b5bf11815..e34d1b00c1 100644 --- a/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php +++ b/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php @@ -141,4 +141,40 @@ public function deleteByMediaUpload(PresentationMediaUpload $mediaUpload): int 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::STATUS_PENDING, + PendingMediaUpload::STATUS_PROCESSING, + PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED, + PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED, + ]) + ->getSingleScalarResult(); + + return intval($count) > 0; + } } diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php new file mode 100644 index 0000000000..4465196450 --- /dev/null +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -0,0 +1,383 @@ +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(IPresentationMediaUploadRepository::class), + App::make(IPendingMediaUploadRepository::class), + App::make(ILockManagerService::class), + App::make(ITransactionService::class) + ); + } + + /** Holds an upload's storage lock, as processPendingMediaUploads does, while $fn runs. */ + private function withStorageLockHeld(int $media_upload_id, \Closure $fn): void + { + $locks = App::make(ILockManagerService::class); + $name = CopyClonedMediaUploadFilesJob::storageLockName($media_upload_id); + $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')); + } + + 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(); + + $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + + $this->assertFalse(Storage::disk('local')->exists( + sprintf('%s/deck.pptx', $target->getPath(IStorageTypesConstants::PrivateType)) + )); + } + + 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 + } + + 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->getId(), 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))); + } + }); + } + + public function testFailedKeepsUploadWhenTargetStorageLocked(): void + { + [$source, $target] = $this->makePair(); + $target_id = $target->getId(); + + $this->withStorageLockHeld($target_id, function () use ($source, $target_id) { + (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + }); + + $this->assertNotNull($this->findUpload($target_id)); + } + + 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))); + } +} From 071a4d11ce480ca687bcf5f905cf66f6fb4e7283 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 14:15:12 -0700 Subject: [PATCH 03/13] fix(media-uploads): clean up a timed-out clone copy under its own lock; make the filename-guard test real --- app/Jobs/CopyClonedMediaUploadFilesJob.php | 62 ++++++++++++++++----- tests/CopyClonedMediaUploadFilesJobTest.php | 48 +++++++++++++++- 2 files changed, 94 insertions(+), 16 deletions(-) diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php index 65af5a7751..de38724f11 100644 --- a/app/Jobs/CopyClonedMediaUploadFilesJob.php +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -56,6 +56,14 @@ final class CopyClonedMediaUploadFilesJob implements ShouldQueue // database-fallback worker's --timeout, so a slow attempt is never re-served concurrently public $timeout = 1200; + // Keyed by lock name, set for the duration handle() holds that lock. On 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 lock - its + // finally never runs before the process is killed. failed() runs on a freshly unserialized + // job instance, so an instance property can't carry the token across to it; a static can, + // since it lives in the same process. See failed() for how this is consumed. + private static array $held_lock_tokens = []; + public function __construct( public int $source_media_upload_id, public int $target_media_upload_id, @@ -76,17 +84,19 @@ public function handle( { Log::debug(sprintf("CopyClonedMediaUploadFilesJob::handle source %s target %s", $this->source_media_upload_id, $this->target_media_upload_id)); + $lock_name = self::storageLockName($this->target_media_upload_id); // UnacquiredLockException propagates: the processor is writing this upload, so retry later - $lock_service->lock( - self::storageLockName($this->target_media_upload_id), - function () use ($repository, $pending_repository, $tx_service) { - $legs = $this->resolveLegs($repository, $pending_repository, $tx_service); - foreach ($legs as $leg) { - $this->copyLeg($leg['storage'], $leg['from'], $leg['to'], $leg['options']); - } - }, - self::STORAGE_LOCK_LIFETIME - ); + $token = $lock_service->acquireLock($lock_name, self::STORAGE_LOCK_LIFETIME); + self::$held_lock_tokens[$lock_name] = $token; + try { + $legs = $this->resolveLegs($repository, $pending_repository, $tx_service); + foreach ($legs as $leg) { + $this->copyLeg($leg['storage'], $leg['from'], $leg['to'], $leg['options']); + } + } finally { + unset(self::$held_lock_tokens[$lock_name]); + $lock_service->releaseLock($lock_name, $token); + } } /** @@ -172,14 +182,40 @@ public function failed(\Throwable $exception): void $this->source_media_upload_id, $this->target_media_upload_id, $exception->getMessage() )); + $lock_service = App::make(ILockManagerService::class); + $lock_name = self::storageLockName($this->target_media_upload_id); + + if (array_key_exists($lock_name, self::$held_lock_tokens)) { + // A final-attempt timeout calls failed() from the worker's SIGALRM handler in the + // SAME process, with handle() 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_lock_tokens[$lock_name]; + Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed target %s copy timed out while still holding its own storage lock, removing the clone's upload", $this->target_media_upload_id)); + try { + $this->removeTarget(); + } 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("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); + Log::error($ex); + } finally { + unset(self::$held_lock_tokens[$lock_name]); + $lock_service->releaseLock($lock_name, $token); + } + return; + } + try { - App::make(ILockManagerService::class)->lock( - self::storageLockName($this->target_media_upload_id), + $lock_service->lock( + $lock_name, fn () => $this->removeTarget(), self::STORAGE_LOCK_LIFETIME ); } catch (UnacquiredLockException $ex) { - // only the processor writing a re-upload of this clone holds the lock, so keep the record + // the lock is held by another process; a same-process timeout is handled above, so + // this can only be the processor writing a re-upload of this clone - keep the record Log::warning(sprintf("CopyClonedMediaUploadFilesJob::failed target %s storage is locked by a re-upload, keeping it", $this->target_media_upload_id)); } catch (\Throwable $ex) { // the job is already terminal: this error log is the operator's signal that the clone diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php index 4465196450..9b4d19f808 100644 --- a/tests/CopyClonedMediaUploadFilesJobTest.php +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -248,12 +248,13 @@ public function testHandleIsNoOpWhenTargetFilenameChanged(): void $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')); - $this->assertFalse(Storage::disk('local')->exists( - sprintf('%s/deck.pptx', $target->getPath(IStorageTypesConstants::PrivateType)) - )); + $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 @@ -355,6 +356,47 @@ public function testFailedKeepsUploadWhenTargetStorageLocked(): void $this->assertNotNull($this->findUpload($target_id)); } + /** + * 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 = CopyClonedMediaUploadFilesJob::storageLockName($target_id); + $locks = App::make(ILockManagerService::class); + $token = $locks->acquireLock($lock_name, 60); + + $held = new \ReflectionProperty(CopyClonedMediaUploadFilesJob::class, 'held_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(); From 4340408c7bce2bc2b1dded61048316876c7ecf90 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 14:22:49 -0700 Subject: [PATCH 04/13] fix(media-uploads): serialize pending upload writes with clone copies via a per-upload lock --- app/Services/Model/Imp/SummitService.php | 26 +++++++++++++ tests/SummitServiceTest.php | 48 ++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index 7688a316dc..a760435e74 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; @@ -54,6 +55,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 +256,11 @@ final class SummitService */ private $cache_service; + /** + * @var ILockManagerService + */ + private $lock_service; + /** * @param ISummitRepository $summit_repository * @param ISummitEventRepository $event_repository @@ -280,6 +288,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 +319,7 @@ public function __construct IEncryptionAES256KeysGenerator $encryption_key_generator, IMUXApi $mux_api, ICacheService $cache_service, + ILockManagerService $lock_service, ITransactionService $tx_service ) { @@ -340,6 +350,7 @@ public function __construct $this->download_strategy = $download_strategy; $this->mux_api = $mux_api; $this->cache_service = $cache_service; + $this->lock_service = $lock_service; } /** @@ -4437,6 +4448,18 @@ public function processPendingMediaUploads(int $max_retries = 3): array continue; } + // The same per-upload lock CopyClonedMediaUploadFilesJob 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. + $lock_name = CopyClonedMediaUploadFilesJob::storageLockName($pending_upload->getMediaUpload()->getId()); + try { + $lock_token = $this->lock_service->acquireLock($lock_name, CopyClonedMediaUploadFilesJob::STORAGE_LOCK_LIFETIME); + } catch (UnacquiredLockException $lock_ex) { + Log::info(sprintf("SummitService::processPendingMediaUploads upload ID %s skipped, media upload storage is locked", $upload_id)); + continue; + } + + try { try { // Mark as Processing and increment attempts $this->tx_service->transaction(function () use ($pending_upload) { @@ -4550,6 +4573,9 @@ public function processPendingMediaUploads(int $max_retries = 3): array $ex->getMessage() )); } + } finally { + $this->lock_service->releaseLock($lock_name, $lock_token); + } } // Cleanup completed uploads older than 7 days diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index f2c2fd1b37..d626588b26 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -20,6 +20,11 @@ use models\summit\SummitEventType; use App\Models\Foundation\Summit\Events\SummitEventTypeConstants; use services\model\ISummitService; +use App\Jobs\CopyClonedMediaUploadFilesJob; +use App\Services\Utils\ILockManagerService; +use models\summit\PendingMediaUpload; +use LaravelDoctrine\ORM\Facades\Registry; +use models\utils\SilverstripeBaseModel; /** * Class SummitServiceTest @@ -342,4 +347,47 @@ 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(\App\Models\Utils\IStorageTypesConstants::PublicType)); + $pending->setPrivatePath($upload->getPath(\App\Models\Utils\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 = CopyClonedMediaUploadFilesJob::storageLockName($upload->getId()); + $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()); + } } From e0bc5634b39bccdd7cfd59bda2ad3ecfac410d36 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 14:39:20 -0700 Subject: [PATCH 05/13] fix(events): copy media upload files when cloning an activity --- app/Services/Model/Imp/SummitService.php | 29 +++++- tests/SummitServiceTest.php | 115 +++++++++++++++++++++++ 2 files changed, 142 insertions(+), 2 deletions(-) diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index a760435e74..a47f5bd16b 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -32,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; @@ -2118,7 +2119,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)) @@ -2266,13 +2269,35 @@ 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]) { + try { + JobDispatcher::withDbFallback( + job: new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), $target->getFilename()), + 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 run failed; its + // failed() has already removed the clone's upload, and the clone is committed + Log::error(sprintf("SummitService::cloneEvent copy of media upload %s for clone %s failed: %s", $target->getId(), $event_clone->getId(), $ex->getMessage())); + } + } + + return $event_clone; } /** diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index d626588b26..b352a92942 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -25,6 +25,15 @@ use models\summit\PendingMediaUpload; use LaravelDoctrine\ORM\Facades\Registry; use models\utils\SilverstripeBaseModel; +use App\Models\Foundation\Summit\Repositories\IPendingMediaUploadRepository; +use App\Models\Foundation\Summit\Repositories\IPresentationMediaUploadRepository; +use App\Models\Utils\IStorageTypesConstants; +use Illuminate\Support\Facades\Bus; +use libs\utils\ITransactionService; +use models\summit\Presentation; +use models\summit\PresentationMediaUpload; +use models\summit\SummitMediaFileType; +use models\summit\SummitMediaUploadType; /** * Class SummitServiceTest @@ -390,4 +399,110 @@ public function testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsL App::make(ISummitService::class)->processPendingMediaUploads(); $this->assertSame(1, $reload()->getAttempts()); } + + /** + * 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(IPresentationMediaUploadRepository::class), + App::make(IPendingMediaUploadRepository::class), + App::make(\App\Services\Utils\ILockManagerService::class), + App::make(ITransactionService::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))); + } + + public function testCloneEventWithoutMediaUploadsQueuesNothing(): void + { + Bus::fake(); + $presentation = $this->presentationWithoutMaterials(); + + App::make(ISummitService::class)->cloneEvent(self::$summit, $presentation->getId()); + + Bus::assertNotDispatched(CopyClonedMediaUploadFilesJob::class); + } } From ccdeb7d69da0b5ff17a3876d3346eb4a7c3944b7 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 14:49:57 -0700 Subject: [PATCH 06/13] test(media-uploads): give processPendingMediaUploads unit tests the storage lock dependency --- tests/SummitServiceTest.php | 4 ++ .../ProcessPendingMediaUploadsTest.php | 68 ++++++++++++++++++- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index b352a92942..ce92e527ce 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -398,6 +398,10 @@ public function testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsL // 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)); } /** diff --git a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php index 436bf97e23..43cffe1ead 100644 --- a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php +++ b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php @@ -13,6 +13,8 @@ **/ 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\SummitService; @@ -20,6 +22,7 @@ use Mockery; use models\summit\ISummitRepository; use models\summit\PendingMediaUpload; +use models\summit\PresentationMediaUpload; use models\summit\Summit; use models\summit\SummitMediaUploadType; use PHPUnit\Framework\TestCase; @@ -71,7 +74,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 +90,17 @@ private function createServiceWithDeps( $prop->setValue($service, $summitRepository); } + // Default: a lock that is always free, so tests that don't care about locking + // behavior exercise the per-row processing exactly as before Task 3. + 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')) { @@ -142,8 +157,12 @@ public function testTransientFailureKeepsUploadPendingForRetry(): void $txService = Mockery::mock(ITransactionService::class); $summitRepository = Mockery::mock(ISummitRepository::class); + $mediaUpload = Mockery::mock(PresentationMediaUpload::class); + $mediaUpload->shouldReceive('getId')->andReturn(101); + $pendingUpload = Mockery::mock(PendingMediaUpload::class); $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($mediaUpload); // 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 +335,12 @@ public function testPartialStatusPreservedOnFailure(): void $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); $txService = Mockery::mock(ITransactionService::class); + $mediaUpload = Mockery::mock(PresentationMediaUpload::class); + $mediaUpload->shouldReceive('getId')->andReturn(102); + $pendingUpload = Mockery::mock(PendingMediaUpload::class); $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($mediaUpload); // 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 +365,47 @@ 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 CopyClonedMediaUploadFilesJob), 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 = Mockery::mock(PresentationMediaUpload::class); + $mediaUpload->shouldReceive('getId')->andReturn(103); + + $pendingUpload = Mockery::mock(PendingMediaUpload::class); + $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($mediaUpload); + $pendingUpload->shouldReceive('getAttempts')->andReturn(0); + $pendingUpload->shouldNotReceive('setStatus'); + $pendingUpload->shouldNotReceive('incrementAttempts'); + $pendingUpload->shouldNotReceive('setErrorMessage'); + + $lockService = Mockery::mock(ILockManagerService::class); + $lockService->shouldReceive('acquireLock')->once()->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']); + } } From 118d4639da7e36e9b49dc10b4ddce10bc6203f52 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 15:08:27 -0700 Subject: [PATCH 07/13] fix(media-uploads): key the clone storage lock on the storage location --- app/Jobs/CopyClonedMediaUploadFilesJob.php | 49 +++++++++++-- app/Services/Model/Imp/SummitService.php | 10 ++- tests/CopyClonedMediaUploadFilesJobTest.php | 35 +++++++-- tests/SummitServiceTest.php | 2 +- .../ProcessPendingMediaUploadsTest.php | 71 +++++++++++++++++-- 5 files changed, 145 insertions(+), 22 deletions(-) diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php index de38724f11..595571e187 100644 --- a/app/Jobs/CopyClonedMediaUploadFilesJob.php +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -37,8 +37,10 @@ * 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). The check and the writes run under the same - * per-upload storage lock SummitService::processPendingMediaUploads takes, so a re-upload that - * lands mid-copy is written after this job, never overwritten by it. + * storage lock SummitService::processPendingMediaUploads takes, so a re-upload that lands mid-copy + * is written after this job, never overwritten by it. The lock is 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. * @package App\Jobs */ final class CopyClonedMediaUploadFilesJob implements ShouldQueue @@ -70,9 +72,10 @@ public function __construct( public string $filename ) {} - public static function storageLockName(int $media_upload_id): string + // the filename is hashed so an arbitrary filename is always a safe cache key + public static function storageLockName(int $presentation_id, string $filename): string { - return sprintf('media_upload_storage.%s', $media_upload_id); + return sprintf('media_upload_storage.%s.%s', $presentation_id, sha1($filename)); } public function handle( @@ -84,8 +87,12 @@ public function handle( { Log::debug(sprintf("CopyClonedMediaUploadFilesJob::handle source %s target %s", $this->source_media_upload_id, $this->target_media_upload_id)); - $lock_name = self::storageLockName($this->target_media_upload_id); - // UnacquiredLockException propagates: the processor is writing this upload, so retry later + $lock_name = $this->resolveLockName($repository, $tx_service); + if (is_null($lock_name)) { + Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s gone or has no presentation, nothing to copy", $this->target_media_upload_id)); + return; + } + // UnacquiredLockException propagates: the processor is writing this location, so retry later $token = $lock_service->acquireLock($lock_name, self::STORAGE_LOCK_LIFETIME); self::$held_lock_tokens[$lock_name] = $token; try { @@ -99,6 +106,21 @@ public function handle( } } + /** + * 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. + */ + private function resolveLockName(IPresentationMediaUploadRepository $repository, ITransactionService $tx_service): ?string + { + return $tx_service->transaction(function () use ($repository) { + $target = $repository->getById($this->target_media_upload_id); + if (!$target instanceof PresentationMediaUpload) return null; + $presentation = $target->getPresentation(); + if (is_null($presentation)) return null; + return self::storageLockName($presentation->getId(), $this->filename); + }); + } + /** * Resolved inside a short read transaction, under the lock; the copy runs outside the transaction. */ @@ -183,7 +205,20 @@ public function failed(\Throwable $exception): void )); $lock_service = App::make(ILockManagerService::class); - $lock_name = self::storageLockName($this->target_media_upload_id); + try { + $lock_name = $this->resolveLockName( + App::make(IPresentationMediaUploadRepository::class), + App::make(ITransactionService::class) + ); + } catch (\Throwable $ex) { + Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); + Log::error($ex); + return; + } + if (is_null($lock_name)) { + Log::info(sprintf("CopyClonedMediaUploadFilesJob::failed target %s gone or has no presentation, nothing to remove", $this->target_media_upload_id)); + return; + } if (array_key_exists($lock_name, self::$held_lock_tokens)) { // A final-attempt timeout calls failed() from the worker's SIGALRM handler in the diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index a47f5bd16b..5188b94a41 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -4473,10 +4473,16 @@ public function processPendingMediaUploads(int $max_retries = 3): array continue; } - // The same per-upload lock CopyClonedMediaUploadFilesJob holds while it writes a + // The same storage-location lock CopyClonedMediaUploadFilesJob 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. - $lock_name = CopyClonedMediaUploadFilesJob::storageLockName($pending_upload->getMediaUpload()->getId()); + $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 = CopyClonedMediaUploadFilesJob::storageLockName($presentation->getId(), $pending_upload->getFileName()); try { $lock_token = $this->lock_service->acquireLock($lock_name, CopyClonedMediaUploadFilesJob::STORAGE_LOCK_LIFETIME); } catch (UnacquiredLockException $lock_ex) { diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php index 9b4d19f808..0f46aee103 100644 --- a/tests/CopyClonedMediaUploadFilesJobTest.php +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -137,10 +137,10 @@ private function runJob(CopyClonedMediaUploadFilesJob $job): void } /** Holds an upload's storage lock, as processPendingMediaUploads does, while $fn runs. */ - private function withStorageLockHeld(int $media_upload_id, \Closure $fn): void + private function withStorageLockHeld(PresentationMediaUpload $upload, \Closure $fn): void { $locks = App::make(ILockManagerService::class); - $name = CopyClonedMediaUploadFilesJob::storageLockName($media_upload_id); + $name = CopyClonedMediaUploadFilesJob::storageLockName($upload->getPresentation()->getId(), $upload->getFilename()); $token = $locks->acquireLock($name, 60); try { $fn(); @@ -334,7 +334,7 @@ public function testHandleThrowsWhenTargetStorageLocked(): void [$source, $target] = $this->makePair(); $this->seedSource($source); - $this->withStorageLockHeld($target->getId(), function () use ($source, $target) { + $this->withStorageLockHeld($target, function () use ($source, $target) { try { $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); $this->fail('expected UnacquiredLockException'); @@ -344,12 +344,37 @@ public function testHandleThrowsWhenTargetStorageLocked(): void }); } + /** + * 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 = CopyClonedMediaUploadFilesJob::storageLockName(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(); - $this->withStorageLockHeld($target_id, function () use ($source, $target_id) { + $this->withStorageLockHeld($target, function () use ($source, $target_id) { (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); }); @@ -371,7 +396,7 @@ public function testFailedAfterTimeoutRemovesCloneUploadUnderItsOwnLock(): void $source_id = $source->getId(); $target_id = $target->getId(); - $lock_name = CopyClonedMediaUploadFilesJob::storageLockName($target_id); + $lock_name = CopyClonedMediaUploadFilesJob::storageLockName(self::$clonePresentation->getId(), 'deck.pptx'); $locks = App::make(ILockManagerService::class); $token = $locks->acquireLock($lock_name, 60); diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index ce92e527ce..01d014e0a8 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -382,7 +382,7 @@ public function testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsL }; $locks = App::make(ILockManagerService::class); - $name = CopyClonedMediaUploadFilesJob::storageLockName($upload->getId()); + $name = CopyClonedMediaUploadFilesJob::storageLockName($presentation->getId(), $upload->getFilename()); $token = $locks->acquireLock($name, 60); try { App::make(ISummitService::class)->processPendingMediaUploads(); diff --git a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php index 43cffe1ead..3151611343 100644 --- a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php +++ b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php @@ -12,6 +12,7 @@ * limitations under the License. **/ +use App\Jobs\CopyClonedMediaUploadFilesJob; use App\Models\Foundation\Summit\Repositories\IPendingMediaUploadRepository; use App\Services\Utils\Exceptions\UnacquiredLockException; use App\Services\Utils\ILockManagerService; @@ -22,6 +23,7 @@ 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; @@ -115,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. @@ -157,12 +169,12 @@ public function testTransientFailureKeepsUploadPendingForRetry(): void $txService = Mockery::mock(ITransactionService::class); $summitRepository = Mockery::mock(ISummitRepository::class); - $mediaUpload = Mockery::mock(PresentationMediaUpload::class); - $mediaUpload->shouldReceive('getId')->andReturn(101); + $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]); @@ -335,12 +347,12 @@ public function testPartialStatusPreservedOnFailure(): void $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); $txService = Mockery::mock(ITransactionService::class); - $mediaUpload = Mockery::mock(PresentationMediaUpload::class); - $mediaUpload->shouldReceive('getId')->andReturn(102); + $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]); @@ -377,19 +389,21 @@ public function testLockedUploadIsSkippedWithoutTouchingIt(): void $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); $txService = Mockery::mock(ITransactionService::class); - $mediaUpload = Mockery::mock(PresentationMediaUpload::class); - $mediaUpload->shouldReceive('getId')->andReturn(103); + $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()->andThrow(new UnacquiredLockException('upload 1 storage is locked')); + $lockService->shouldReceive('acquireLock')->once() + ->with(CopyClonedMediaUploadFilesJob::storageLockName(103, 'deck.pptx'), CopyClonedMediaUploadFilesJob::STORAGE_LOCK_LIFETIME) + ->andThrow(new UnacquiredLockException('upload 1 storage is locked')); $lockService->shouldNotReceive('releaseLock'); $pendingRepo->shouldReceive('resetStuckProcessingRows')->once()->with(10)->andReturn(0); @@ -408,4 +422,47 @@ public function testLockedUploadIsSkippedWithoutTouchingIt(): void $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']); + } } From 53d6140d6bd8bb1ea36de8ea2651c7d564abdc91 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 15:10:32 -0700 Subject: [PATCH 08/13] fix(media-uploads): copy the legs a source has instead of failing the whole clone upload --- app/Jobs/CopyClonedMediaUploadFilesJob.php | 33 +++++++++++++++++---- tests/CopyClonedMediaUploadFilesJobTest.php | 16 ++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php index 595571e187..4071a2ca97 100644 --- a/app/Jobs/CopyClonedMediaUploadFilesJob.php +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -97,9 +97,18 @@ public function handle( self::$held_lock_tokens[$lock_name] = $token; try { $legs = $this->resolveLegs($repository, $pending_repository, $tx_service); + $enabled = 0; + $copied = 0; foreach ($legs as $leg) { - $this->copyLeg($leg['storage'], $leg['from'], $leg['to'], $leg['options']); + $result = $this->copyLeg($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("CopyClonedMediaUploadFilesJob source media upload %s has no file on any storage", $this->source_media_upload_id)); } finally { unset(self::$held_lock_tokens[$lock_name]); $lock_service->releaseLock($lock_name, $token); @@ -154,12 +163,14 @@ private function resolveLegs( 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), @@ -175,18 +186,29 @@ private function isReplaced(PresentationMediaUpload $target, IPendingMediaUpload || $pending_repository->hasAnyForMediaUpload($target->getId()); } - private function copyLeg(?string $storage, string $from, string $to, $options): void + /** + * @return bool|null null when the leg's storage is disabled, false when its source file is + * missing (skipped), true when copied + */ + private function copyLeg(string $kind, ?string $storage, string $from, string $to, $options): ?bool { - if (empty($storage)) return; + if (empty($storage)) return null; $download = FileDownloadStrategyFactory::build($storage); $upload = FileUploadStrategyFactory::build($storage); - if (is_null($download) || is_null($upload)) return; // None + 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("CopyClonedMediaUploadFilesJob::copyLeg %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("CopyClonedMediaUploadFilesJob source missing on %s: %s", $storage, $from)); + throw new \RuntimeException(sprintf("CopyClonedMediaUploadFilesJob could not read on %s: %s", $storage, $from)); try { if (!$upload->saveFromStream($stream, $to, $options)) @@ -195,6 +217,7 @@ private function copyLeg(?string $storage, string $from, string $to, $options): if (is_resource($stream)) fclose($stream); } Log::info(sprintf("CopyClonedMediaUploadFilesJob::copyLeg %s copied %s -> %s", $storage, $from, $to)); + return true; } public function failed(\Throwable $exception): void diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php index 0f46aee103..47ee647474 100644 --- a/tests/CopyClonedMediaUploadFilesJobTest.php +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -230,6 +230,22 @@ public function testHandleThrowsWhenSourceFileMissing(): void $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(); From 03674abf3c9eec30e64113ce16141dbe39b4c386 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 15:12:54 -0700 Subject: [PATCH 09/13] fix(media-uploads): lock the clone row on removal, log lock misses without a cause, cover the dispatch-failure catch --- app/Jobs/CopyClonedMediaUploadFilesJob.php | 11 ++++++--- tests/CopyClonedMediaUploadFilesJobTest.php | 27 +++++++++++++++++++++ tests/SummitServiceTest.php | 25 +++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php index 4071a2ca97..d9ef4b1e06 100644 --- a/app/Jobs/CopyClonedMediaUploadFilesJob.php +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -272,9 +272,10 @@ public function failed(\Throwable $exception): void self::STORAGE_LOCK_LIFETIME ); } catch (UnacquiredLockException $ex) { - // the lock is held by another process; a same-process timeout is handled above, so - // this can only be the processor writing a re-upload of this clone - keep the record - Log::warning(sprintf("CopyClonedMediaUploadFilesJob::failed target %s storage is locked by a re-upload, keeping it", $this->target_media_upload_id)); + // 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("CopyClonedMediaUploadFilesJob::failed 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", $this->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) @@ -291,7 +292,9 @@ private function removeTarget(): 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 = App::make(ITransactionService::class)->transaction(function () use ($repository, $pending_repository) { - $target = $repository->getById($this->target_media_upload_id); + // 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 = $repository->getByIdExclusiveLock($this->target_media_upload_id); if (!$target instanceof PresentationMediaUpload) return []; if ($this->isReplaced($target, $pending_repository)) { Log::info(sprintf("CopyClonedMediaUploadFilesJob::failed target %s was re-uploaded, keeping it", $this->target_media_upload_id)); diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php index 47ee647474..309a31baf9 100644 --- a/tests/CopyClonedMediaUploadFilesJobTest.php +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -20,6 +20,7 @@ use App\Services\Utils\ILockManagerService; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use libs\utils\ITransactionService; use models\summit\PendingMediaUpload; @@ -319,6 +320,27 @@ public function testFailedRemovesCloneUploadAndKeepsOriginal(): void $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); + + (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(); @@ -390,11 +412,16 @@ 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')); } /** diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index 01d014e0a8..1c1cd95954 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -500,6 +500,31 @@ public function testCloneEventQueuesCopyAndCopyLandsFiles(): void $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()); + } + public function testCloneEventWithoutMediaUploadsQueuesNothing(): void { Bus::fake(); From cd34c68ac6e41647cf424fb018238f5a1c892254 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 17:02:46 -0700 Subject: [PATCH 10/13] fix(media-uploads): clean up a clone upload when its copy cannot be dispatched; match getPath's empty-title rule in the backfill --- app/Services/Model/Imp/SummitService.php | 16 +++++++++++--- .../model/Version20261014120001.php | 2 +- tests/SummitServiceTest.php | 22 +++++++++++++++++++ ...FrozenPathSegmentBackfillMigrationTest.php | 11 ++++++++++ 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index 5188b94a41..def38e99cf 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -2284,16 +2284,26 @@ public function cloneEvent(Summit $summit, int $event_id): SummitEvent // 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: new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), $target->getFilename()), + 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 run failed; its - // failed() has already removed the clone's upload, and the clone is committed + // 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()); call failed() explicitly so the clone's upload + // is cleaned up instead of pointing at a file that was never copied. failed() is + // idempotent and never throws, 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())); + try { + $job->failed($ex); + } catch (\Throwable $cleanup_ex) { + Log::error(sprintf("SummitService::cloneEvent failed() cleanup for media upload %s on clone %s threw: %s", $target->getId(), $event_clone->getId(), $cleanup_ex->getMessage())); + } } } diff --git a/database/migrations/model/Version20261014120001.php b/database/migrations/model/Version20261014120001.php index 750d21c38c..b9e72e8123 100644 --- a/database/migrations/model/Version20261014120001.php +++ b/database/migrations/model/Version20261014120001.php @@ -41,7 +41,7 @@ public function up(Schema $schema): void foreach ($rows as $row) { $title = (string)($row['Title'] ?? ''); - $slug = $title !== '' ? Presentation::slugify($title) : $row['PresentationSlug']; + $slug = !empty($title) ? Presentation::slugify($title) : $row['PresentationSlug']; $segment = PresentationMediaUpload::buildModernPathSegment( intval($row['SummitID']), $row['SummitSlug'], intval($row['PresentationID']), $slug ); diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index 1c1cd95954..e1d814f918 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -523,6 +523,28 @@ public function testCloneEventStillReturnsCloneWhenCopyDispatchFails(): void $this->assertInstanceOf(Presentation::class, $clone); $this->assertGreaterThan(0, $clone->getId()); $this->assertNotSame($presentation->getId(), $clone->getId()); + + // By the time cloneEvent returns, the explicit failed() 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', + \Illuminate\Support\Facades\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 diff --git a/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php b/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php index 12327f7f55..0c1c72af3f 100644 --- a/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php +++ b/tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php @@ -67,6 +67,17 @@ public function testFallsBackToStoredSlugWhenTitleEmpty(): void $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([]); From 41e7104b752c8ec0233d336b9a3ddce2b0a99d99 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 17:32:32 -0700 Subject: [PATCH 11/13] refactor(media-uploads): move clone copy and cleanup into SummitService --- app/Jobs/CopyClonedMediaUploadFilesJob.php | 285 +-------- app/Services/Model/ISummitService.php | 30 + app/Services/Model/Imp/SummitService.php | 546 ++++++++++++++---- tests/CopyClonedMediaUploadFilesJobTest.php | 34 +- tests/SummitServiceTest.php | 30 +- .../ProcessPendingMediaUploadsTest.php | 8 +- 6 files changed, 503 insertions(+), 430 deletions(-) diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php index d9ef4b1e06..88751b6083 100644 --- a/app/Jobs/CopyClonedMediaUploadFilesJob.php +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -12,212 +12,46 @@ * limitations under the License. **/ -use App\Models\Foundation\Summit\Repositories\IPendingMediaUploadRepository; -use App\Models\Foundation\Summit\Repositories\IPresentationMediaUploadRepository; -use App\Models\Utils\IStorageTypesConstants; -use App\Services\Filesystem\FileDownloadStrategyFactory; -use App\Services\Filesystem\FileUploadStrategyFactory; -use App\Services\Utils\Exceptions\UnacquiredLockException; -use App\Services\Utils\ILockManagerService; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Log; -use libs\utils\ITransactionService; -use models\summit\PresentationMediaUpload; +use services\model\ISummitService; /** * Copies a cloned media upload's files from the original's storage paths to the clone's. * SummitService::cloneEvent copies the record only; the clone's path is built from the clone's * presentation ID, where nothing exists until this job runs. If every try fails, failed() - * deletes the clone's record so it never points at a missing file. - * - * 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). The check and the writes run under the same - * storage lock SummitService::processPendingMediaUploads takes, so a re-upload that lands mid-copy - * is written after this job, never overwritten by it. The lock is 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. + * removes the clone's record so it never points at a missing file. + * See SummitService::copyClonedMediaUploadFiles and SummitService::removeUncopiedClonedMediaUpload. * @package App\Jobs */ final class CopyClonedMediaUploadFilesJob implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable; - // above $timeout, so a killed attempt's lock expires on its own before anything waits on it long - const STORAGE_LOCK_LIFETIME = 1300; - public $tries = 5; public $backoff = [30, 60, 120, 300]; // strictly below every queue connection's retry_after (1800, config/queue.php) and the - // database-fallback worker's --timeout, so a slow attempt is never re-served concurrently + // database-fallback worker's --timeout, so a slow attempt is never re-served concurrently; + // ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME stays above it public $timeout = 1200; - // Keyed by lock name, set for the duration handle() holds that lock. On 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 lock - its - // finally never runs before the process is killed. failed() runs on a freshly unserialized - // job instance, so an instance property can't carry the token across to it; a static can, - // since it lives in the same process. See failed() for how this is consumed. - private static array $held_lock_tokens = []; - public function __construct( - public int $source_media_upload_id, - public int $target_media_upload_id, - public string $filename + public readonly int $source_media_upload_id, + public readonly int $target_media_upload_id, + public readonly string $filename ) {} - // the filename is hashed so an arbitrary filename is always a safe cache key - public static function storageLockName(int $presentation_id, string $filename): string - { - return sprintf('media_upload_storage.%s.%s', $presentation_id, sha1($filename)); - } - - public function handle( - IPresentationMediaUploadRepository $repository, - IPendingMediaUploadRepository $pending_repository, - ILockManagerService $lock_service, - ITransactionService $tx_service - ): void + public function handle(ISummitService $service): void { Log::debug(sprintf("CopyClonedMediaUploadFilesJob::handle source %s target %s", $this->source_media_upload_id, $this->target_media_upload_id)); - $lock_name = $this->resolveLockName($repository, $tx_service); - if (is_null($lock_name)) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s gone or has no presentation, nothing to copy", $this->target_media_upload_id)); - return; - } - // UnacquiredLockException propagates: the processor is writing this location, so retry later - $token = $lock_service->acquireLock($lock_name, self::STORAGE_LOCK_LIFETIME); - self::$held_lock_tokens[$lock_name] = $token; - try { - $legs = $this->resolveLegs($repository, $pending_repository, $tx_service); - $enabled = 0; - $copied = 0; - foreach ($legs as $leg) { - $result = $this->copyLeg($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("CopyClonedMediaUploadFilesJob source media upload %s has no file on any storage", $this->source_media_upload_id)); - } finally { - unset(self::$held_lock_tokens[$lock_name]); - $lock_service->releaseLock($lock_name, $token); - } - } - - /** - * 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. - */ - private function resolveLockName(IPresentationMediaUploadRepository $repository, ITransactionService $tx_service): ?string - { - return $tx_service->transaction(function () use ($repository) { - $target = $repository->getById($this->target_media_upload_id); - if (!$target instanceof PresentationMediaUpload) return null; - $presentation = $target->getPresentation(); - if (is_null($presentation)) return null; - return self::storageLockName($presentation->getId(), $this->filename); - }); - } - - /** - * Resolved inside a short read transaction, under the lock; the copy runs outside the transaction. - */ - private function resolveLegs( - IPresentationMediaUploadRepository $repository, - IPendingMediaUploadRepository $pending_repository, - ITransactionService $tx_service - ): array - { - return $tx_service->transaction(function () use ($repository, $pending_repository) { - $target = $repository->getById($this->target_media_upload_id); - if (!$target instanceof PresentationMediaUpload) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s gone, nothing to copy", $this->target_media_upload_id)); - return []; - } - if ($this->isReplaced($target, $pending_repository)) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s was re-uploaded, nothing to copy", $this->target_media_upload_id)); - return []; - } - $source = $repository->getById($this->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("CopyClonedMediaUploadFilesJob source media upload %s is gone", $this->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 ($pending_repository->hasInFlightForMediaUpload($source->getId())) - throw new \RuntimeException(sprintf("CopyClonedMediaUploadFilesJob source media upload %s has a re-upload in flight", $this->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', - ], - ]; - }); - } - - private function isReplaced(PresentationMediaUpload $target, IPendingMediaUploadRepository $pending_repository): bool - { - return $target->getFilename() !== $this->filename - || $pending_repository->hasAnyForMediaUpload($target->getId()); - } - - /** - * @return bool|null null when the leg's storage is disabled, false when its source file is - * missing (skipped), true when copied - */ - private function copyLeg(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("CopyClonedMediaUploadFilesJob::copyLeg %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("CopyClonedMediaUploadFilesJob could not read on %s: %s", $storage, $from)); - - try { - if (!$upload->saveFromStream($stream, $to, $options)) - throw new \RuntimeException(sprintf("CopyClonedMediaUploadFilesJob could not write on %s: %s", $storage, $to)); - } finally { - if (is_resource($stream)) fclose($stream); - } - Log::info(sprintf("CopyClonedMediaUploadFilesJob::copyLeg %s copied %s -> %s", $storage, $from, $to)); - return true; + $service->copyClonedMediaUploadFiles($this->source_media_upload_id, $this->target_media_upload_id, $this->filename); } public function failed(\Throwable $exception): void @@ -226,108 +60,13 @@ public function failed(\Throwable $exception): void "CopyClonedMediaUploadFilesJob::failed source %s target %s: %s", $this->source_media_upload_id, $this->target_media_upload_id, $exception->getMessage() )); - - $lock_service = App::make(ILockManagerService::class); - try { - $lock_name = $this->resolveLockName( - App::make(IPresentationMediaUploadRepository::class), - App::make(ITransactionService::class) - ); - } catch (\Throwable $ex) { - Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); - Log::error($ex); - return; - } - if (is_null($lock_name)) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::failed target %s gone or has no presentation, nothing to remove", $this->target_media_upload_id)); - return; - } - - if (array_key_exists($lock_name, self::$held_lock_tokens)) { - // A final-attempt timeout calls failed() from the worker's SIGALRM handler in the - // SAME process, with handle() 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_lock_tokens[$lock_name]; - Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed target %s copy timed out while still holding its own storage lock, removing the clone's upload", $this->target_media_upload_id)); - try { - $this->removeTarget(); - } 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("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); - Log::error($ex); - } finally { - unset(self::$held_lock_tokens[$lock_name]); - $lock_service->releaseLock($lock_name, $token); - } - return; - } + Log::error($exception); try { - $lock_service->lock( - $lock_name, - fn () => $this->removeTarget(), - self::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("CopyClonedMediaUploadFilesJob::failed 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", $this->target_media_upload_id)); + App::make(ISummitService::class)->removeUncopiedClonedMediaUpload($this->target_media_upload_id, $this->filename, $exception); } 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("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); Log::error($ex); } } - - private function removeTarget(): void - { - $repository = App::make(IPresentationMediaUploadRepository::class); - $pending_repository = App::make(IPendingMediaUploadRepository::class); - - // 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 = App::make(ITransactionService::class)->transaction(function () use ($repository, $pending_repository) { - // 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 = $repository->getByIdExclusiveLock($this->target_media_upload_id); - if (!$target instanceof PresentationMediaUpload) return []; - if ($this->isReplaced($target, $pending_repository)) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::failed target %s was re-uploaded, keeping it", $this->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("CopyClonedMediaUploadFilesJob::failed removed clone media upload %s", $this->target_media_upload_id)); - return $paths; - }); - - // same rename-to-DELETED_ cleanup as PresentationService::deleteMediaUpload - foreach ($cleanup as [$storage, $path, $filename]) { - try { - $strategy = FileUploadStrategyFactory::build($storage); - if (!is_null($strategy)) $strategy->markAsDeleted($path, $filename); - } catch (\Throwable $ex) { - Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not mark %s/%s deleted on %s", $path, $filename, $storage)); - Log::error($ex); - } - } - } } diff --git a/app/Services/Model/ISummitService.php b/app/Services/Model/ISummitService.php index c7c9b16849..467ca00db4 100644 --- a/app/Services/Model/ISummitService.php +++ b/app/Services/Model/ISummitService.php @@ -41,6 +41,11 @@ */ 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 + const MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME = 1300; + /** * @param Summit $summit * @param array $data @@ -557,6 +562,31 @@ 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 + * @param \Throwable|null $cause why the copy failed + */ + public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename, ?\Throwable $cause = null): 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 def38e99cf..b1e27605a5 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -262,6 +262,14 @@ final class SummitService */ 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 @@ -2294,15 +2302,17 @@ public function cloneEvent(Summit $summit, int $event_id): SummitEvent } 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()); call failed() explicitly so the clone's upload - // is cleaned up instead of pointing at a file that was never copied. failed() is - // idempotent and never throws, but guard the call anyway so cloneEvent still returns - // the already-committed clone. + // 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 { - $job->failed($ex); + $this->removeUncopiedClonedMediaUpload($target->getId(), $target->getFilename(), $ex); } catch (\Throwable $cleanup_ex) { - Log::error(sprintf("SummitService::cloneEvent failed() cleanup for media upload %s on clone %s threw: %s", $target->getId(), $event_clone->getId(), $cleanup_ex->getMessage())); + 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); } } } @@ -4483,7 +4493,7 @@ public function processPendingMediaUploads(int $max_retries = 3): array continue; } - // The same storage-location lock CopyClonedMediaUploadFilesJob holds while it writes a + // 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(); @@ -4492,128 +4502,16 @@ public function processPendingMediaUploads(int $max_retries = 3): array Log::warning(sprintf("SummitService::processPendingMediaUploads upload ID %s skipped, media upload %s has no presentation", $upload_id, $pending_upload->getMediaUpload()->getId())); continue; } - $lock_name = CopyClonedMediaUploadFilesJob::storageLockName($presentation->getId(), $pending_upload->getFileName()); + $lock_name = self::mediaUploadStorageLockName($presentation->getId(), $pending_upload->getFileName()); try { - $lock_token = $this->lock_service->acquireLock($lock_name, CopyClonedMediaUploadFilesJob::STORAGE_LOCK_LIFETIME); + $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; } try { - 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() - )); - } + $this->processPendingUpload($pending_upload, $upload_id, $max_retries, $stats); } finally { $this->lock_service->releaseLock($lock_name, $lock_token); } @@ -4644,4 +4542,408 @@ 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("CopyClonedMediaUploadFilesJob::handle 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("CopyClonedMediaUploadFilesJob 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, ?\Throwable $cause = null): void + { + Log::debug(sprintf( + "SummitService::removeUncopiedClonedMediaUpload target %s cause %s", + $target_media_upload_id, is_null($cause) ? 'none' : $cause->getMessage() + )); + + try { + $lock_name = $this->resolveClonedMediaUploadLockName($target_media_upload_id, $filename); + } catch (\Throwable $ex) { + Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $target_media_upload_id)); + Log::error($ex); + return; + } + if (is_null($lock_name)) { + Log::info(sprintf("CopyClonedMediaUploadFilesJob::failed 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("CopyClonedMediaUploadFilesJob::failed 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("CopyClonedMediaUploadFilesJob::failed 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("CopyClonedMediaUploadFilesJob::failed 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("CopyClonedMediaUploadFilesJob::failed 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("CopyClonedMediaUploadFilesJob::handle target %s gone, nothing to copy", $target_media_upload_id)); + return []; + } + if ($this->isClonedMediaUploadReplaced($target, $filename)) { + Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle 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("CopyClonedMediaUploadFilesJob 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("CopyClonedMediaUploadFilesJob 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("CopyClonedMediaUploadFilesJob::copyLeg %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("CopyClonedMediaUploadFilesJob could not read on %s: %s", $storage, $from)); + + try { + if (!$upload->saveFromStream($stream, $to, $options)) + throw new \RuntimeException(sprintf("CopyClonedMediaUploadFilesJob could not write on %s: %s", $storage, $to)); + } finally { + if (is_resource($stream)) fclose($stream); + } + Log::info(sprintf("CopyClonedMediaUploadFilesJob::copyLeg %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("CopyClonedMediaUploadFilesJob::failed 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("CopyClonedMediaUploadFilesJob::failed 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("CopyClonedMediaUploadFilesJob::failed could not mark %s/%s deleted on %s", $path, $stored_filename, $storage)); + Log::error($ex); + } + } + } } diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php index 309a31baf9..d4098330b8 100644 --- a/tests/CopyClonedMediaUploadFilesJobTest.php +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -13,7 +13,6 @@ **/ use App\Jobs\CopyClonedMediaUploadFilesJob; -use App\Models\Foundation\Summit\Repositories\IPendingMediaUploadRepository; use App\Models\Foundation\Summit\Repositories\IPresentationMediaUploadRepository; use App\Models\Utils\IStorageTypesConstants; use App\Services\Utils\Exceptions\UnacquiredLockException; @@ -22,15 +21,18 @@ use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; -use libs\utils\ITransactionService; use models\summit\PendingMediaUpload; use models\summit\Presentation; use models\summit\PresentationMediaUpload; use models\summit\SummitMediaFileType; use models\summit\SummitMediaUploadType; +use services\model\ISummitService; +use services\model\SummitService; /** - * DB-backed: the job reloads both uploads by ID. Storage is the Local driver on a faked disk. + * DB-backed: SummitService::copyClonedMediaUploadFiles / removeUncopiedClonedMediaUpload, run + * through the job's handle()/failed(), reload both uploads by ID. Storage is the Local driver on a + * faked disk. */ class CopyClonedMediaUploadFilesJobTest extends TestCase { @@ -129,19 +131,14 @@ private function markReuploaded(PresentationMediaUpload $upload, string $status private function runJob(CopyClonedMediaUploadFilesJob $job): void { - $job->handle( - App::make(IPresentationMediaUploadRepository::class), - App::make(IPendingMediaUploadRepository::class), - App::make(ILockManagerService::class), - App::make(ITransactionService::class) - ); + $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 = CopyClonedMediaUploadFilesJob::storageLockName($upload->getPresentation()->getId(), $upload->getFilename()); + $name = SummitService::mediaUploadStorageLockName($upload->getPresentation()->getId(), $upload->getFilename()); $token = $locks->acquireLock($name, 60); try { $fn(); @@ -166,6 +163,14 @@ public function testQueueSettingsStayBelowRetryAfter(): void $this->assertLessThan(Config::get('queue.connections.database.retry_after'), $job->timeout); } + public function testHandleDelegatesToSummitService(): void + { + $service = \Mockery::mock(ISummitService::class); + $service->shouldReceive('copyClonedMediaUploadFiles')->once()->with(1, 2, 'deck.pptx'); + + (new CopyClonedMediaUploadFilesJob(1, 2, 'deck.pptx'))->handle($service); + } + public function testHandleCopiesBothLegsAndLeavesOriginal(): void { [$source, $target] = $this->makePair(); @@ -335,6 +340,9 @@ public function testFailedLoadsTargetUnderExclusiveLock(): void $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')); @@ -393,7 +401,7 @@ public function testHandleThrowsWhenSameLocationLockedByDifferentUpload(): void $locks = App::make(ILockManagerService::class); // the name a processor writing a DIFFERENT upload record at the clone's deck.pptx would hold - $name = CopyClonedMediaUploadFilesJob::storageLockName(self::$clonePresentation->getId(), 'deck.pptx'); + $name = SummitService::mediaUploadStorageLockName(self::$clonePresentation->getId(), 'deck.pptx'); $token = $locks->acquireLock($name, 60); try { $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); @@ -439,11 +447,11 @@ public function testFailedAfterTimeoutRemovesCloneUploadUnderItsOwnLock(): void $source_id = $source->getId(); $target_id = $target->getId(); - $lock_name = CopyClonedMediaUploadFilesJob::storageLockName(self::$clonePresentation->getId(), 'deck.pptx'); + $lock_name = SummitService::mediaUploadStorageLockName(self::$clonePresentation->getId(), 'deck.pptx'); $locks = App::make(ILockManagerService::class); $token = $locks->acquireLock($lock_name, 60); - $held = new \ReflectionProperty(CopyClonedMediaUploadFilesJob::class, 'held_lock_tokens'); + $held = new \ReflectionProperty(SummitService::class, 'held_media_upload_lock_tokens'); $held->setAccessible(true); $held->setValue(null, [$lock_name => $token]); diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index e1d814f918..75bd5900cd 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -21,19 +21,18 @@ use App\Models\Foundation\Summit\Events\SummitEventTypeConstants; use services\model\ISummitService; use App\Jobs\CopyClonedMediaUploadFilesJob; -use App\Services\Utils\ILockManagerService; -use models\summit\PendingMediaUpload; -use LaravelDoctrine\ORM\Facades\Registry; -use models\utils\SilverstripeBaseModel; -use App\Models\Foundation\Summit\Repositories\IPendingMediaUploadRepository; -use App\Models\Foundation\Summit\Repositories\IPresentationMediaUploadRepository; use App\Models\Utils\IStorageTypesConstants; +use App\Services\Utils\ILockManagerService; use Illuminate\Support\Facades\Bus; -use libs\utils\ITransactionService; +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 @@ -366,8 +365,8 @@ public function testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsL $pending->setSummitId(self::$summit->getId()); $pending->setMediaUploadTypeId($upload->getMediaUploadType()->getId()); $pending->setMediaUpload($upload); - $pending->setPublicPath($upload->getPath(\App\Models\Utils\IStorageTypesConstants::PublicType)); - $pending->setPrivatePath($upload->getPath(\App\Models\Utils\IStorageTypesConstants::PrivateType)); + $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); @@ -382,7 +381,7 @@ public function testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsL }; $locks = App::make(ILockManagerService::class); - $name = CopyClonedMediaUploadFilesJob::storageLockName($presentation->getId(), $upload->getFilename()); + $name = SummitService::mediaUploadStorageLockName($presentation->getId(), $upload->getFilename()); $token = $locks->acquireLock($name, 60); try { App::make(ISummitService::class)->processPendingMediaUploads(); @@ -487,12 +486,7 @@ public function testCloneEventQueuesCopyAndCopyLandsFiles(): void }); $job = Bus::dispatched(CopyClonedMediaUploadFilesJob::class)->first(); - $job->handle( - App::make(IPresentationMediaUploadRepository::class), - App::make(IPendingMediaUploadRepository::class), - App::make(\App\Services\Utils\ILockManagerService::class), - App::make(ITransactionService::class) - ); + $job->handle(App::make(ISummitService::class)); $disk = Storage::disk('local'); $this->assertSame('private-bytes', $disk->get($clone_upload->getRelativePath(IStorageTypesConstants::PrivateType))); @@ -524,7 +518,7 @@ public function testCloneEventStillReturnsCloneWhenCopyDispatchFails(): void $this->assertGreaterThan(0, $clone->getId()); $this->assertNotSame($presentation->getId(), $clone->getId()); - // By the time cloneEvent returns, the explicit failed() call in its catch has already run + // 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 @@ -532,7 +526,7 @@ public function testCloneEventStillReturnsCloneWhenCopyDispatchFails(): void $clone_id = $clone->getId(); $clone_private_path = sprintf( '%s/%s/%s/%s/%s', - \Illuminate\Support\Facades\Config::get('mediaupload.private_mounting_folder'), + Config::get('mediaupload.private_mounting_folder'), IStorageTypesConstants::PrivateType, self::$summit->getId(), $clone_id, diff --git a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php index 3151611343..f48accc5cc 100644 --- a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php +++ b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php @@ -12,12 +12,12 @@ * limitations under the License. **/ -use App\Jobs\CopyClonedMediaUploadFilesJob; 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; @@ -93,7 +93,7 @@ private function createServiceWithDeps( } // Default: a lock that is always free, so tests that don't care about locking - // behavior exercise the per-row processing exactly as before Task 3. + // exercise only the per-row processing. if (!$lockService) { $lockService = Mockery::mock(ILockManagerService::class); $lockService->shouldReceive('acquireLock')->andReturn('test-token'); @@ -380,7 +380,7 @@ public function testPartialStatusPreservedOnFailure(): void /** * Unit-level counterpart of SummitServiceTest::testProcessPendingMediaUploadsSkipsUploadWhileCloneCopyHoldsItsLock. - * When the upload's storage lock is held (e.g. by CopyClonedMediaUploadFilesJob), the row + * 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. */ @@ -402,7 +402,7 @@ public function testLockedUploadIsSkippedWithoutTouchingIt(): void $lockService = Mockery::mock(ILockManagerService::class); $lockService->shouldReceive('acquireLock')->once() - ->with(CopyClonedMediaUploadFilesJob::storageLockName(103, 'deck.pptx'), CopyClonedMediaUploadFilesJob::STORAGE_LOCK_LIFETIME) + ->with(SummitService::mediaUploadStorageLockName(103, 'deck.pptx'), ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME) ->andThrow(new UnacquiredLockException('upload 1 storage is locked')); $lockService->shouldNotReceive('releaseLock'); From 140ea40b606fd18a1d80bb8e9c74f1ca69ab0e56 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 17:32:33 -0700 Subject: [PATCH 12/13] style(media-uploads): align clone copy code with repo conventions --- .../Foundation/Summit/PendingMediaUpload.php | 8 ++++++++ .../DoctrinePendingMediaUploadRepository.php | 14 ++------------ .../migrations/model/Version20261014120000.php | 7 ++++++- .../migrations/model/Version20261014120001.php | 7 ++++++- 4 files changed, 22 insertions(+), 14 deletions(-) diff --git a/app/Models/Foundation/Summit/PendingMediaUpload.php b/app/Models/Foundation/Summit/PendingMediaUpload.php index d9a1b1aa2e..44347855d2 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/Repositories/Summit/DoctrinePendingMediaUploadRepository.php b/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php index e34d1b00c1..46e13a69db 100644 --- a/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php +++ b/app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php @@ -132,12 +132,7 @@ 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(); } @@ -167,12 +162,7 @@ public function hasInFlightForMediaUpload(int $media_upload_id): bool "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::STATUS_PENDING, - PendingMediaUpload::STATUS_PROCESSING, - PendingMediaUpload::STATUS_PUBLIC_STORAGE_UPLOADED, - PendingMediaUpload::STATUS_PRIVATE_STORAGE_UPLOADED, - ]) + ->setParameter('statuses', PendingMediaUpload::IN_FLIGHT_STATUSES) ->getSingleScalarResult(); return intval($count) > 0; diff --git a/database/migrations/model/Version20261014120000.php b/database/migrations/model/Version20261014120000.php index 1ed14cc2a2..4a994ef885 100644 --- a/database/migrations/model/Version20261014120000.php +++ b/database/migrations/model/Version20261014120000.php @@ -20,8 +20,13 @@ * Adds PresentationMediaUpload.FrozenPathSegment. Backfilled by Version20261014120001. * @package Database\Migrations\Model */ -class Version20261014120000 extends AbstractMigration +final class Version20261014120000 extends AbstractMigration { + public function getDescription(): string + { + return 'Add PresentationMediaUpload.FrozenPathSegment.'; + } + /** * @param Schema $schema */ diff --git a/database/migrations/model/Version20261014120001.php b/database/migrations/model/Version20261014120001.php index b9e72e8123..18266f33bd 100644 --- a/database/migrations/model/Version20261014120001.php +++ b/database/migrations/model/Version20261014120001.php @@ -25,8 +25,13 @@ * title is empty (generateSlug() returns early on an empty title). * @package Database\Migrations\Model */ -class Version20261014120001 extends AbstractMigration +final class Version20261014120001 extends AbstractMigration { + public function getDescription(): string + { + return 'Backfill PresentationMediaUpload.FrozenPathSegment for modern-format media uploads.'; + } + public function up(Schema $schema): void { $rows = $this->connection->fetchAllAssociative(<< Date: Wed, 14 Oct 2026 17:43:53 -0700 Subject: [PATCH 13/13] refactor(media-uploads): name service log lines after SummitService; trim the clone cleanup interface --- app/Jobs/CopyClonedMediaUploadFilesJob.php | 2 +- app/Services/Model/ISummitService.php | 5 ++- app/Services/Model/Imp/SummitService.php | 47 +++++++++------------ tests/CopyClonedMediaUploadFilesJobTest.php | 8 ---- 4 files changed, 25 insertions(+), 37 deletions(-) diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php index 88751b6083..d2efa3b2a0 100644 --- a/app/Jobs/CopyClonedMediaUploadFilesJob.php +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -63,7 +63,7 @@ public function failed(\Throwable $exception): void Log::error($exception); try { - App::make(ISummitService::class)->removeUncopiedClonedMediaUpload($this->target_media_upload_id, $this->filename, $exception); + 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/Services/Model/ISummitService.php b/app/Services/Model/ISummitService.php index 467ca00db4..0068ad5cec 100644 --- a/app/Services/Model/ISummitService.php +++ b/app/Services/Model/ISummitService.php @@ -44,6 +44,8 @@ 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; /** @@ -583,9 +585,8 @@ public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $tar * * @param int $target_media_upload_id * @param string $filename the clone's filename when the copy was queued - * @param \Throwable|null $cause why the copy failed */ - public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename, ?\Throwable $cause = null): void; + public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename): void; /** * @param Summit $summit diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index b1e27605a5..435f367e8b 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -2309,7 +2309,7 @@ public function cloneEvent(Summit $summit, int $event_id): SummitEvent 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(), $ex); + $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); @@ -4689,7 +4689,7 @@ public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $tar { $lock_name = $this->resolveClonedMediaUploadLockName($target_media_upload_id, $filename); if (is_null($lock_name)) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s gone or has no presentation, nothing to copy", $target_media_upload_id)); + 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 @@ -4708,7 +4708,7 @@ public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $tar // 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("CopyClonedMediaUploadFilesJob source media upload %s has no file on any storage", $source_media_upload_id)); + 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); @@ -4718,22 +4718,17 @@ public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $tar /** * @inheritDoc */ - public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename, ?\Throwable $cause = null): void + public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename): void { - Log::debug(sprintf( - "SummitService::removeUncopiedClonedMediaUpload target %s cause %s", - $target_media_upload_id, is_null($cause) ? 'none' : $cause->getMessage() - )); - try { $lock_name = $this->resolveClonedMediaUploadLockName($target_media_upload_id, $filename); } catch (\Throwable $ex) { - Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $target_media_upload_id)); + 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("CopyClonedMediaUploadFilesJob::failed target %s gone or has no presentation, nothing to remove", $target_media_upload_id)); + Log::info(sprintf("SummitService::removeUncopiedClonedMediaUpload target %s gone or has no presentation, nothing to remove", $target_media_upload_id)); return; } @@ -4744,13 +4739,13 @@ public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, str // 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("CopyClonedMediaUploadFilesJob::failed target %s copy timed out while still holding its own storage lock, removing the clone's upload", $target_media_upload_id)); + 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("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $target_media_upload_id)); + 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]); @@ -4769,11 +4764,11 @@ public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, str // 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("CopyClonedMediaUploadFilesJob::failed 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)); + 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("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $target_media_upload_id)); + Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload could not remove clone media upload %s", $target_media_upload_id)); Log::error($ex); } } @@ -4808,21 +4803,21 @@ private function resolveClonedMediaUploadLegs(int $source_media_upload_id, int $ 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("CopyClonedMediaUploadFilesJob::handle target %s gone, nothing to copy", $target_media_upload_id)); + Log::info(sprintf("SummitService::resolveClonedMediaUploadLegs target %s gone, nothing to copy", $target_media_upload_id)); return []; } if ($this->isClonedMediaUploadReplaced($target, $filename)) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::handle target %s was re-uploaded, nothing to copy", $target_media_upload_id)); + 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("CopyClonedMediaUploadFilesJob source media upload %s is gone", $source_media_upload_id)); + 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("CopyClonedMediaUploadFilesJob source media upload %s has a re-upload in flight", $source_media_upload_id)); + 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 []; @@ -4879,7 +4874,7 @@ private function copyClonedMediaUploadLeg(string $kind, ?string $storage, string // 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("CopyClonedMediaUploadFilesJob::copyLeg %s leg source missing on %s, skipping it: %s", $kind, $storage, $from)); + Log::warning(sprintf("SummitService::copyClonedMediaUploadLeg %s leg source missing on %s, skipping it: %s", $kind, $storage, $from)); return false; } @@ -4887,15 +4882,15 @@ private function copyClonedMediaUploadLeg(string $kind, ?string $storage, string // 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("CopyClonedMediaUploadFilesJob could not read on %s: %s", $storage, $from)); + 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("CopyClonedMediaUploadFilesJob could not write on %s: %s", $storage, $to)); + throw new \RuntimeException(sprintf("SummitService::copyClonedMediaUploadLeg could not write on %s: %s", $storage, $to)); } finally { if (is_resource($stream)) fclose($stream); } - Log::info(sprintf("CopyClonedMediaUploadFilesJob::copyLeg %s copied %s -> %s", $storage, $from, $to)); + Log::info(sprintf("SummitService::copyClonedMediaUploadLeg %s copied %s -> %s", $storage, $from, $to)); return true; } @@ -4913,7 +4908,7 @@ private function removeClonedMediaUploadTarget(int $target_media_upload_id, stri $target = $this->presentation_media_upload_repository->getByIdExclusiveLock($target_media_upload_id); if (!$target instanceof PresentationMediaUpload) return []; if ($this->isClonedMediaUploadReplaced($target, $filename)) { - Log::info(sprintf("CopyClonedMediaUploadFilesJob::failed target %s was re-uploaded, keeping it", $target_media_upload_id)); + Log::info(sprintf("SummitService::removeClonedMediaUploadTarget target %s was re-uploaded, keeping it", $target_media_upload_id)); return []; } @@ -4931,7 +4926,7 @@ private function removeClonedMediaUploadTarget(int $target_media_upload_id, stri $presentation = $target->getPresentation(); if (!is_null($presentation)) $presentation->removeMediaUpload($target); - Log::warning(sprintf("CopyClonedMediaUploadFilesJob::failed removed clone media upload %s", $target_media_upload_id)); + Log::warning(sprintf("SummitService::removeClonedMediaUploadTarget removed clone media upload %s", $target_media_upload_id)); return $paths; }); @@ -4941,7 +4936,7 @@ private function removeClonedMediaUploadTarget(int $target_media_upload_id, stri $strategy = FileUploadStrategyFactory::build($storage); if (!is_null($strategy)) $strategy->markAsDeleted($path, $stored_filename); } catch (\Throwable $ex) { - Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not mark %s/%s deleted on %s", $path, $stored_filename, $storage)); + Log::error(sprintf("SummitService::removeClonedMediaUploadTarget could not mark %s/%s deleted on %s", $path, $stored_filename, $storage)); Log::error($ex); } } diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php index d4098330b8..d0cd6f31db 100644 --- a/tests/CopyClonedMediaUploadFilesJobTest.php +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -163,14 +163,6 @@ public function testQueueSettingsStayBelowRetryAfter(): void $this->assertLessThan(Config::get('queue.connections.database.retry_after'), $job->timeout); } - public function testHandleDelegatesToSummitService(): void - { - $service = \Mockery::mock(ISummitService::class); - $service->shouldReceive('copyClonedMediaUploadFiles')->once()->with(1, 2, 'deck.pptx'); - - (new CopyClonedMediaUploadFilesJob(1, 2, 'deck.pptx'))->handle($service); - } - public function testHandleCopiesBothLegsAndLeavesOriginal(): void { [$source, $target] = $this->makePair();