From 84846fc913b6aa101abb54049ab3bb3e11eb1743 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Wed, 14 Oct 2026 13:51:01 -0700 Subject: [PATCH 01/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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(); From 0b9476cdeef45746255439656baaa13857ea86f4 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Fri, 13 Nov 2026 18:34:27 -0600 Subject: [PATCH 14/14] fix(media-uploads): lock the source during a clone copy, read records fresh, keep uploads flowing without the lock store - hold the source location's storage lock as well as the target's, so a copy never reads an original its processor or another copy is still writing - carry both presentation IDs in the job so the locks are taken before any database read, and read both records past the identity map under the locks - copy from the filename the clone was queued with, not the source's current one - mark the copied files deleted when the clone's upload is deleted mid-copy - give the upload processor its own lock lifetime, matched to the stuck-row reset, and size the copy job's backoff to outlast a held storage lock - process pending uploads without the lock when the lock store is unavailable, and never let a lock error abort the rest of the batch --- app/Jobs/CopyClonedMediaUploadFilesJob.php | 17 +- .../IPresentationMediaUploadRepository.php | 9 + ...trinePresentationMediaUploadRepository.php | 16 ++ app/Services/Model/ISummitService.php | 31 ++- app/Services/Model/Imp/SummitService.php | 246 ++++++++++++------ tests/CopyClonedMediaUploadFilesJobTest.php | 198 +++++++++++--- tests/SummitServiceTest.php | 4 +- .../ProcessPendingMediaUploadsTest.php | 111 +++++++- 8 files changed, 497 insertions(+), 135 deletions(-) diff --git a/app/Jobs/CopyClonedMediaUploadFilesJob.php b/app/Jobs/CopyClonedMediaUploadFilesJob.php index d2efa3b2a0..4d2321c8f8 100644 --- a/app/Jobs/CopyClonedMediaUploadFilesJob.php +++ b/app/Jobs/CopyClonedMediaUploadFilesJob.php @@ -32,9 +32,12 @@ final class CopyClonedMediaUploadFilesJob implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable; - public $tries = 5; + // the backoff adds up to more than ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME, so a copy + // waiting on a storage lock (another copy, the upload processor, or one left by a killed run) + // is still retrying when that lock is released or expires + public $tries = 8; - public $backoff = [30, 60, 120, 300]; + public $backoff = [30, 60, 120, 300, 300, 300, 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; @@ -43,7 +46,9 @@ final class CopyClonedMediaUploadFilesJob implements ShouldQueue public function __construct( public readonly int $source_media_upload_id, + public readonly int $source_presentation_id, public readonly int $target_media_upload_id, + public readonly int $target_presentation_id, public readonly string $filename ) {} @@ -51,7 +56,11 @@ 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)); - $service->copyClonedMediaUploadFiles($this->source_media_upload_id, $this->target_media_upload_id, $this->filename); + $service->copyClonedMediaUploadFiles( + $this->source_media_upload_id, $this->source_presentation_id, + $this->target_media_upload_id, $this->target_presentation_id, + $this->filename + ); } public function failed(\Throwable $exception): void @@ -63,7 +72,7 @@ public function failed(\Throwable $exception): void Log::error($exception); try { - App::make(ISummitService::class)->removeUncopiedClonedMediaUpload($this->target_media_upload_id, $this->filename); + App::make(ISummitService::class)->removeUncopiedClonedMediaUpload($this->target_media_upload_id, $this->target_presentation_id, $this->filename); } catch (\Throwable $ex) { Log::error(sprintf("CopyClonedMediaUploadFilesJob::failed could not remove clone media upload %s", $this->target_media_upload_id)); Log::error($ex); diff --git a/app/Models/Foundation/Summit/Repositories/IPresentationMediaUploadRepository.php b/app/Models/Foundation/Summit/Repositories/IPresentationMediaUploadRepository.php index adff27e851..7f437bb792 100644 --- a/app/Models/Foundation/Summit/Repositories/IPresentationMediaUploadRepository.php +++ b/app/Models/Foundation/Summit/Repositories/IPresentationMediaUploadRepository.php @@ -13,9 +13,18 @@ **/ +use models\summit\PresentationMediaUpload; use models\utils\IBaseRepository; interface IPresentationMediaUploadRepository extends IBaseRepository { + /** + * Reads the row from the database even when the entity is already managed, so a row another + * process deleted or changed is seen as it is now. Null when the row is gone. + * @param int $id + * @param bool $exclusive_lock lock the row FOR UPDATE; needs an open transaction + * @return PresentationMediaUpload|null + */ + public function getByIdFresh(int $id, bool $exclusive_lock = false): ?PresentationMediaUpload; } \ No newline at end of file diff --git a/app/Repositories/Summit/DoctrinePresentationMediaUploadRepository.php b/app/Repositories/Summit/DoctrinePresentationMediaUploadRepository.php index 1d9422f2b1..7002285b5f 100644 --- a/app/Repositories/Summit/DoctrinePresentationMediaUploadRepository.php +++ b/app/Repositories/Summit/DoctrinePresentationMediaUploadRepository.php @@ -14,6 +14,8 @@ use App\Models\Foundation\Summit\Repositories\IPresentationMediaUploadRepository; use App\Repositories\SilverStripeDoctrineRepository; +use Doctrine\DBAL\LockMode; +use Doctrine\ORM\Query; use Doctrine\ORM\QueryBuilder; use models\summit\PresentationMediaUpload; use utils\Filter; @@ -62,4 +64,18 @@ protected function applyExtraJoins(QueryBuilder $query, ?Filter $filter = null, $query = $query->innerJoin("e.media_upload_type", "t"); return $query; } + + /** + * @inheritDoc + */ + public function getByIdFresh(int $id, bool $exclusive_lock = false): ?PresentationMediaUpload + { + // find() answers from the identity map without a query; a refreshing DQL read does not + $query = $this->getEntityManager() + ->createQuery(sprintf('SELECT e FROM %s e WHERE e.id = :id', PresentationMediaUpload::class)) + ->setParameter('id', $id) + ->setHint(Query::HINT_REFRESH, true); + if ($exclusive_lock) $query->setLockMode(LockMode::PESSIMISTIC_WRITE); + return $query->getOneOrNullResult(); + } } \ No newline at end of file diff --git a/app/Services/Model/ISummitService.php b/app/Services/Model/ISummitService.php index 0068ad5cec..08fcfd6a35 100644 --- a/app/Services/Model/ISummitService.php +++ b/app/Services/Model/ISummitService.php @@ -41,13 +41,20 @@ */ 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 + // lifetime of the media upload storage locks (SummitService::mediaUploadStorageLockName) a clone + // copy holds; above CopyClonedMediaUploadFilesJob::$timeout, so a killed copy's locks expire on + // their own, and below the job's total backoff, so a waiting copy outlasts them // 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; + // minutes after which processPendingMediaUploads treats a Processing row as left by a crashed run + const PENDING_MEDIA_UPLOAD_STUCK_MINUTES = 10; + + // lifetime of the storage lock processPendingMediaUploads holds per row: a killed run's lock + // expires by the time its stuck row is reset, so the row is never reset yet still locked + const PENDING_MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME = self::PENDING_MEDIA_UPLOAD_STUCK_MINUTES * 60; + /** * @param Summit $summit * @param array $data @@ -565,18 +572,23 @@ 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 + * Copies a cloned media upload's files from the original's storage paths to the clone's, holding + * the storage locks of both locations (the ones processPendingMediaUploads takes), so neither + * side is written while it is read or copied. The presentation IDs come from the caller so the + * locks are taken before anything is read from the database. 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. + * on every enabled leg throws. If the clone's upload is deleted while the copy runs, the copied + * files are marked deleted. * * @param int $source_media_upload_id + * @param int $source_presentation_id * @param int $target_media_upload_id + * @param int $target_presentation_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 \App\Services\Utils\Exceptions\UnacquiredLockException when either 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; + public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $source_presentation_id, int $target_media_upload_id, int $target_presentation_id, string $filename): void; /** * Removes a cloned media upload whose files could not be copied, so the clone never points at a @@ -584,9 +596,10 @@ public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $tar * for the operator, not thrown. * * @param int $target_media_upload_id + * @param int $target_presentation_id * @param string $filename the clone's filename when the copy was queued */ - public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename): void; + public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, int $target_presentation_id, string $filename): void; /** * @param Summit $summit diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index 435f367e8b..783ac84da3 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -262,11 +262,12 @@ final class SummitService */ private $lock_service; - // Keyed by lock name, set for the duration copyClonedMediaUploadFiles holds that lock. On a + // Keyed by the target's lock name; each value lists the [lock name, token] pairs that copy holds + // (target, then source), set for the duration copyClonedMediaUploadFiles holds them. 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 + // the locks - its finally never runs before the process is killed. A static, not an instance + // property, so the tokens reach removeUncopiedClonedMediaUpload however the service was // resolved; it lives as long as the process does. See removeUncopiedClonedMediaUpload. private static array $held_media_upload_lock_tokens = []; @@ -2292,7 +2293,9 @@ 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()); + $job = new CopyClonedMediaUploadFilesJob( + $source->getId(), $event_id, $target->getId(), $event_clone->getId(), $target->getFilename() + ); try { JobDispatcher::withDbFallback( job: $job, @@ -2309,7 +2312,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()); + $this->removeUncopiedClonedMediaUpload($target->getId(), $event_clone->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); @@ -4464,7 +4467,7 @@ public function processPendingMediaUploads(int $max_retries = 3): array try { // Reset stuck Processing rows (indicates crashed cron run) $this->tx_service->transaction(function () { - $reset_count = $this->pending_media_upload_repository->resetStuckProcessingRows(10); + $reset_count = $this->pending_media_upload_repository->resetStuckProcessingRows(ISummitService::PENDING_MEDIA_UPLOAD_STUCK_MINUTES); if ($reset_count > 0) { Log::warning(sprintf("SummitService::processPendingMediaUploads reset %s stuck Processing rows", $reset_count)); @@ -4493,9 +4496,10 @@ public function processPendingMediaUploads(int $max_retries = 3): array continue; } - // The same storage-location lock copyClonedMediaUploadFiles holds while it writes a - // clone's files. If a copy is running, leave this row untouched for the next run, so - // the staff file is written after the copy and never overwritten by it. + // The same storage-location lock copyClonedMediaUploadFiles holds while it reads an + // original's files or writes a clone's. 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 or read half-written. $presentation = $pending_upload->getMediaUpload()->getPresentation(); if (is_null($presentation)) { // no presentation means no valid path to write to; leave the row untouched @@ -4503,17 +4507,20 @@ public function processPendingMediaUploads(int $max_retries = 3): array continue; } $lock_name = self::mediaUploadStorageLockName($presentation->getId(), $pending_upload->getFileName()); - try { - $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; - } + $lock_token = $this->acquirePendingMediaUploadStorageLock($lock_name, $upload_id); + if ($lock_token === false) continue; try { $this->processPendingUpload($pending_upload, $upload_id, $max_retries, $stats); } finally { - $this->lock_service->releaseLock($lock_name, $lock_token); + if (!is_null($lock_token)) { + try { + $this->lock_service->releaseLock($lock_name, $lock_token); + } catch (\Throwable $release_ex) { + // the lock expires on its own; never let it stop the rest of the batch + Log::warning(sprintf("SummitService::processPendingMediaUploads upload ID %s could not release its storage lock: %s", $upload_id, $release_ex->getMessage())); + } + } } } @@ -4543,6 +4550,37 @@ public function processPendingMediaUploads(int $max_retries = 3): array return $stats; } + /** + * Takes a pending upload's storage lock. Staff uploads must keep flowing when the lock store is + * down, so only a lock that is really held skips the row; a store that cannot answer does not. + * A clone copy takes the same lock and fails closed, and it also checks the PendingMediaUpload + * rows in the database, so a row processed without the lock is still seen by it. + * @param string $lock_name + * @param int $upload_id + * @return string|false|null the token; false when the lock is held (skip the row); null when + * the lock store is unavailable (process the row without the lock) + */ + private function acquirePendingMediaUploadStorageLock(string $lock_name, int $upload_id): string|false|null + { + try { + return $this->lock_service->acquireLock($lock_name, ISummitService::PENDING_MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME); + } catch (UnacquiredLockException $lock_ex) { + // a miss is either a holder or an unreachable store; only a holder leaves the key readable + try { + if ($this->cache_service->exists($lock_name)) { + Log::info(sprintf("SummitService::processPendingMediaUploads upload ID %s skipped, media upload storage is locked", $upload_id)); + return false; + } + } catch (\Throwable $exists_ex) { + Log::warning(sprintf("SummitService::processPendingMediaUploads upload ID %s could not read its storage lock: %s", $upload_id, $exists_ex->getMessage())); + } + } catch (\Throwable $lock_ex) { + Log::warning(sprintf("SummitService::processPendingMediaUploads upload ID %s could not take its storage lock: %s", $upload_id, $lock_ex->getMessage())); + } + Log::warning(sprintf("SummitService::processPendingMediaUploads upload ID %s lock store unavailable, processing without the storage lock", $upload_id)); + return null; + } + /** * Writes one pending upload to public then private storage. Runs under the upload's storage * lock, which processPendingMediaUploads holds and releases. @@ -4685,60 +4723,101 @@ public static function mediaUploadStorageLockName(int $presentation_id, string $ /** * @inheritDoc */ - public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $target_media_upload_id, string $filename): void + public function copyClonedMediaUploadFiles(int $source_media_upload_id, int $source_presentation_id, int $target_media_upload_id, int $target_presentation_id, string $filename): void { - $lock_name = $this->resolveClonedMediaUploadLockName($target_media_upload_id, $filename); - if (is_null($lock_name)) { - Log::info(sprintf("SummitService::copyClonedMediaUploadFiles target %s gone or has no presentation, nothing to copy", $target_media_upload_id)); - return; - } - // UnacquiredLockException propagates: the processor is writing this location, so retry later - $token = $this->lock_service->acquireLock($lock_name, ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME); - self::$held_media_upload_lock_tokens[$lock_name] = $token; + // Both names come from the caller, so nothing is read from the database until the locks are + // held. The source's lock keeps its processor, and a copy still filling it when the source + // is itself a clone, from writing the files while they are read. + $target_lock = self::mediaUploadStorageLockName($target_presentation_id, $filename); + $source_lock = self::mediaUploadStorageLockName($source_presentation_id, $filename); + $lock_names = $source_lock === $target_lock ? [$target_lock] : [$target_lock, $source_lock]; + + $held = []; try { + foreach ($lock_names as $lock_name) { + // UnacquiredLockException propagates: the location is being written, so retry later. + // acquireLock never blocks, so two copies taking these in opposite order cannot deadlock. + $held[] = [$lock_name, $this->lock_service->acquireLock($lock_name, ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME)]; + self::$held_media_upload_lock_tokens[$target_lock] = $held; + } + $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++; + $copied = []; + $copying = null; + try { + foreach ($legs as $leg) { + $copying = $leg; + $result = $this->copyClonedMediaUploadLeg($leg['kind'], $leg['storage'], $leg['from'], $leg['to'], $leg['options']); + $copying = null; + if (is_null($result)) continue; + $enabled++; + if ($result) $copied[] = $leg; + } + } finally { + // staff may have deleted the clone's upload while the legs were copying; a leg that + // threw mid-write may have left a partial file, so it counts as written too + $this->discardCopiedLegsIfTargetGone($target_media_upload_id, $filename, is_null($copying) ? $copied : array_merge($copied, [$copying])); } // a source missing on every enabled leg is a broken original: fail so the retries, // then failed(), resolve it - if ($enabled > 0 && $copied === 0) + if ($enabled > 0 && count($copied) === 0) throw new \RuntimeException(sprintf("SummitService::copyClonedMediaUploadFiles source media upload %s has no file on any storage", $source_media_upload_id)); } finally { - unset(self::$held_media_upload_lock_tokens[$lock_name]); - $this->lock_service->releaseLock($lock_name, $token); + unset(self::$held_media_upload_lock_tokens[$target_lock]); + foreach (array_reverse($held) as [$lock_name, $token]) { + $this->lock_service->releaseLock($lock_name, $token); + } } } /** - * @inheritDoc + * deleteMediaUpload only marks deleted the files that exist when it runs, so legs written after + * it would stay readable at the clone's path with no record. Runs under the target's lock. + * @param int $target_media_upload_id + * @param string $filename + * @param array $copied the legs this copy wrote, or started to write */ - public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, string $filename): void + private function discardCopiedLegsIfTargetGone(int $target_media_upload_id, string $filename, array $copied): void { + if (count($copied) === 0) return; try { - $lock_name = $this->resolveClonedMediaUploadLockName($target_media_upload_id, $filename); + $gone = $this->tx_service->transaction(function () use ($target_media_upload_id) { + return is_null($this->presentation_media_upload_repository->getByIdFresh($target_media_upload_id)); + }); } catch (\Throwable $ex) { - Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload could not remove clone media upload %s", $target_media_upload_id)); + Log::error(sprintf("SummitService::discardCopiedLegsIfTargetGone could not re-check clone media upload %s", $target_media_upload_id)); Log::error($ex); return; } - if (is_null($lock_name)) { - Log::info(sprintf("SummitService::removeUncopiedClonedMediaUpload target %s gone or has no presentation, nothing to remove", $target_media_upload_id)); - return; + if (!$gone) return; + + Log::warning(sprintf("SummitService::discardCopiedLegsIfTargetGone clone media upload %s was deleted during the copy, marking the copied files deleted", $target_media_upload_id)); + foreach ($copied as $leg) { + try { + $strategy = FileUploadStrategyFactory::build($leg['storage']); + if (!is_null($strategy)) $strategy->markAsDeleted($leg['to_path'], $filename); + } catch (\Throwable $ex) { + Log::error(sprintf("SummitService::discardCopiedLegsIfTargetGone could not mark %s deleted on %s", $leg['to'], $leg['storage'])); + Log::error($ex); + } } + } + + /** + * @inheritDoc + */ + public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, int $target_presentation_id, string $filename): void + { + $lock_name = self::mediaUploadStorageLockName($target_presentation_id, $filename); 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]; + // the SAME process, with copyClonedMediaUploadFiles suspended mid-copy still holding its + // locks - its finally never runs before the process is killed. The target's lock is + // still ours: re-acquiring it here would always throw UnacquiredLockException, so clean + // up directly and release the tokens we already hold instead. + $held = self::$held_media_upload_lock_tokens[$lock_name]; Log::error(sprintf("SummitService::removeUncopiedClonedMediaUpload target %s copy timed out while still holding its own storage lock, removing the clone's upload", $target_media_upload_id)); try { $this->removeClonedMediaUploadTarget($target_media_upload_id, $filename); @@ -4749,7 +4828,13 @@ public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, str Log::error($ex); } finally { unset(self::$held_media_upload_lock_tokens[$lock_name]); - $this->lock_service->releaseLock($lock_name, $token); + foreach (array_reverse($held) as [$held_name, $token]) { + try { + $this->lock_service->releaseLock($held_name, $token); + } catch (\Throwable $release_ex) { + Log::error($release_ex); + } + } } return; } @@ -4774,25 +4859,9 @@ public function removeUncopiedClonedMediaUpload(int $target_media_upload_id, str } /** - * 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. + * Resolved inside a short read transaction, under the locks; the copy runs outside the transaction. + * Both records are read fresh: a worker that keeps its EntityManager across attempts would + * otherwise be handed the entities an earlier attempt loaded. * @param int $source_media_upload_id * @param int $target_media_upload_id * @param string $filename @@ -4801,7 +4870,7 @@ private function resolveClonedMediaUploadLockName(int $target_media_upload_id, s 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); + $target = $this->presentation_media_upload_repository->getByIdFresh($target_media_upload_id); if (!$target instanceof PresentationMediaUpload) { Log::info(sprintf("SummitService::resolveClonedMediaUploadLegs target %s gone, nothing to copy", $target_media_upload_id)); return []; @@ -4810,7 +4879,7 @@ private function resolveClonedMediaUploadLegs(int $source_media_upload_id, int $ 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); + $source = $this->presentation_media_upload_repository->getByIdFresh($source_media_upload_id); // the clone still needs this file; fail so the retries, then failed(), resolve it if (!$source instanceof PresentationMediaUpload) throw new \RuntimeException(sprintf("SummitService::resolveClonedMediaUploadLegs source media upload %s is gone", $source_media_upload_id)); @@ -4822,22 +4891,25 @@ private function resolveClonedMediaUploadLegs(int $source_media_upload_id, int $ $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', - ], - ]; + // Both sides use the filename the clone was queued with, never the source's current + // one: a re-upload of the original under a new name leaves the cloned file in place + // beside the new one, and the clone's record still describes the cloned file. + $legs = []; + foreach ([ + IStorageTypesConstants::PrivateType => [$type->getPrivateStorageType(), []], + IStorageTypesConstants::PublicType => [$type->getPublicStorageType(), $type->isUseTemporaryLinksOnPublicStorage() ? [] : 'public'], + ] as $kind => [$storage, $options]) { + $to_path = $target->getPath($kind); + $legs[] = [ + 'kind' => $kind, + 'storage' => $storage, + 'from' => sprintf('%s/%s', $source->getPath($kind), $filename), + 'to_path' => $to_path, + 'to' => sprintf('%s/%s', $to_path, $filename), + 'options' => $options, + ]; + } + return $legs; }); } @@ -4904,8 +4976,10 @@ private function removeClonedMediaUploadTarget(int $target_media_upload_id, stri // 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); + // (or wins, and the guard below sees it) instead of being cascaded away with the record. + // Read fresh, so a record staff already deleted is not answered from an earlier attempt's + // entity and the file a new upload wrote at the same path is left alone. + $target = $this->presentation_media_upload_repository->getByIdFresh($target_media_upload_id, true); if (!$target instanceof PresentationMediaUpload) return []; if ($this->isClonedMediaUploadReplaced($target, $filename)) { Log::info(sprintf("SummitService::removeClonedMediaUploadTarget target %s was re-uploaded, keeping it", $target_media_upload_id)); diff --git a/tests/CopyClonedMediaUploadFilesJobTest.php b/tests/CopyClonedMediaUploadFilesJobTest.php index d0cd6f31db..934380020a 100644 --- a/tests/CopyClonedMediaUploadFilesJobTest.php +++ b/tests/CopyClonedMediaUploadFilesJobTest.php @@ -129,6 +129,24 @@ private function markReuploaded(PresentationMediaUpload $upload, string $status self::$em->flush(); } + private function makeJob(int $source_id, int $target_id): CopyClonedMediaUploadFilesJob + { + return new CopyClonedMediaUploadFilesJob( + $source_id, self::$original->getId(), $target_id, self::$clonePresentation->getId(), 'deck.pptx' + ); + } + + /** + * Deletes an upload's rows behind the EntityManager's back, as another process would: the + * entity this process already loaded stays in the identity map. + */ + private function deleteRowOutsideEntityManager(int $id): void + { + $connection = self::$em->getConnection(); + $connection->executeStatement('DELETE FROM PresentationMediaUpload WHERE ID = ?', [$id]); + $connection->executeStatement('DELETE FROM PresentationMaterial WHERE ID = ?', [$id]); + } + private function runJob(CopyClonedMediaUploadFilesJob $job): void { $job->handle(App::make(ISummitService::class)); @@ -154,13 +172,16 @@ private function findUpload(int $id): ?PresentationMediaUpload ->find(PresentationMediaUpload::class, $id); } - public function testQueueSettingsStayBelowRetryAfter(): void + public function testQueueSettingsKeepTheirOrdering(): void { - $job = new CopyClonedMediaUploadFilesJob(1, 2, 'deck.pptx'); - $this->assertSame(5, $job->tries); - $this->assertSame([30, 60, 120, 300], $job->backoff); + $job = $this->makeJob(1, 2); + // a slow attempt is never re-served while it still runs $this->assertLessThan(Config::get('queue.connections.redis.retry_after'), $job->timeout); $this->assertLessThan(Config::get('queue.connections.database.retry_after'), $job->timeout); + // a killed attempt's locks outlive it, and a copy waiting on a lock outlasts that lock + $this->assertGreaterThan($job->timeout, ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME); + $this->assertCount($job->tries - 1, $job->backoff); + $this->assertGreaterThan(ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME, array_sum($job->backoff)); } public function testHandleCopiesBothLegsAndLeavesOriginal(): void @@ -168,7 +189,7 @@ public function testHandleCopiesBothLegsAndLeavesOriginal(): void [$source, $target] = $this->makePair(); $this->seedSource($source); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $disk = Storage::disk('local'); $this->assertSame('private-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); @@ -187,7 +208,7 @@ public function testHandleCopiesFromModernSourceToLegacyClone(): void $this->seedSource($source); $this->assertStringContainsString('1-frozen/2-frozen-title', $source->getRelativePath(IStorageTypesConstants::PrivateType)); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $expected = sprintf('%s/%s/deck.pptx', self::$summit->getId(), self::$clonePresentation->getId()); $this->assertStringEndsWith($expected, $target->getRelativePath(IStorageTypesConstants::PrivateType)); @@ -200,7 +221,7 @@ public function testHandleOverwritesPartialTargetFromEarlierAttempt(): void $this->seedSource($source); Storage::disk('local')->put($target->getRelativePath(IStorageTypesConstants::PrivateType), 'priv'); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $disk = Storage::disk('local'); $this->assertSame('private-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); @@ -214,7 +235,7 @@ public function testHandleSkipsStorageTypeNone(): 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')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $this->assertSame('private-bytes', Storage::disk('local')->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); $this->assertFalse(Storage::disk('local')->exists($target->getRelativePath(IStorageTypesConstants::PublicType))); @@ -225,7 +246,7 @@ public function testHandleThrowsWhenSourceFileMissing(): void [$source, $target] = $this->makePair(); $this->expectException(\RuntimeException::class); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); } /** @@ -237,7 +258,7 @@ 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')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $disk = Storage::disk('local'); $this->assertSame('private-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); @@ -253,7 +274,7 @@ public function testHandleThrowsWhenSourceRecordGone(): void self::$em->flush(); $this->expectException(\RuntimeException::class); - $this->runJob(new CopyClonedMediaUploadFilesJob($source_id, $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source_id, $target->getId())); } public function testHandleIsNoOpWhenTargetFilenameChanged(): void @@ -264,7 +285,7 @@ public function testHandleIsNoOpWhenTargetFilenameChanged(): void self::$em->flush(); Storage::disk('local')->put($target->getRelativePath(IStorageTypesConstants::PrivateType), 'staff-bytes'); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $disk = Storage::disk('local'); $this->assertSame('staff-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); @@ -278,7 +299,7 @@ public function testHandleIsNoOpWhenTargetReuploadedWithSameFilename(): void $this->markReuploaded($target); Storage::disk('local')->put($target->getRelativePath(IStorageTypesConstants::PrivateType), 'staff-bytes'); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $disk = Storage::disk('local'); $this->assertSame('staff-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); @@ -293,7 +314,7 @@ public function testHandleIsNoOpWhenTargetGone(): void self::$clonePresentation->removeMediaUpload($target); self::$em->flush(); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target_id)); $this->assertNull($this->findUpload($target_id)); } @@ -308,7 +329,7 @@ public function testFailedRemovesCloneUploadAndKeepsOriginal(): void $source_id = $source->getId(); $target_id = $target->getId(); - (new CopyClonedMediaUploadFilesJob($source_id, $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + ($this->makeJob($source_id, $target_id))->failed(new \RuntimeException('boom')); $this->assertNull($this->findUpload($target_id)); $this->assertNotNull($this->findUpload($source_id)); @@ -318,27 +339,72 @@ public function testFailedRemovesCloneUploadAndKeepsOriginal(): void } /** - * 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. + * A worker that keeps its EntityManager across attempts still holds the clone's upload entity + * after staff delete it and add a new upload at the same path. The file there is the new + * upload's, so neither the copy nor the cleanup may touch it. + */ + public function testHandleLeavesFileAloneWhenTargetDeletedByAnotherProcess(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $target_private = $target->getRelativePath(IStorageTypesConstants::PrivateType); + $target_public = $target->getRelativePath(IStorageTypesConstants::PublicType); + Storage::disk('local')->put($target_private, 'staff-bytes'); + $this->deleteRowOutsideEntityManager($target->getId()); + + $this->runJob($this->makeJob($source->getId(), $target->getId())); + + $disk = Storage::disk('local'); + $this->assertSame('staff-bytes', $disk->get($target_private)); + $this->assertFalse($disk->exists($target_public)); + } + + public function testFailedLeavesFileAloneWhenTargetDeletedByAnotherProcess(): void + { + [$source, $target] = $this->makePair(); + $target_private = $target->getRelativePath(IStorageTypesConstants::PrivateType); + Storage::disk('local')->put($target_private, 'staff-bytes'); + $this->deleteRowOutsideEntityManager($target->getId()); + + ($this->makeJob($source->getId(), $target->getId()))->failed(new \RuntimeException('boom')); + + $this->assertSame('staff-bytes', Storage::disk('local')->get($target_private)); + } + + /** + * Staff delete the clone's upload while its legs are copying: deleteMediaUpload has already run, + * so the copy itself must mark what it wrote deleted, or the files stay readable with no record. */ - public function testFailedLoadsTargetUnderExclusiveLock(): void + public function testHandleMarksCopiedFilesDeletedWhenTargetDeletedDuringCopy(): void { [$source, $target] = $this->makePair(); + $this->seedSource($source); + $source_id = $source->getId(); $target_id = $target->getId(); + $target_private = $target->getRelativePath(IStorageTypesConstants::PrivateType); + $target_public = $target->getRelativePath(IStorageTypesConstants::PublicType); $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)); + $repository->shouldReceive('getByIdFresh')->andReturnUsing(function ($id, $exclusive_lock = false) use ($real, $source_id, $target_id) { + $found = $real->getByIdFresh($id, $exclusive_lock); + // the source is the last record read before the legs are copied: delete the clone's + // upload here, after the copy has seen it and before it re-checks + if ($id === $source_id) $this->deleteRowOutsideEntityManager($target_id); + return $found; + }); 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 + // resolved so the job gets one built with the wrapped repository App::forgetInstance(ISummitService::class); - (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + $this->runJob($this->makeJob($source_id, $target_id)); - $this->assertNull($this->findUpload($target_id)); + $disk = Storage::disk('local'); + $this->assertFalse($disk->exists($target_private)); + $this->assertFalse($disk->exists($target_public)); + $this->assertSame('private-bytes', $disk->get(dirname($target_private) . '/DELETED_deck.pptx')); + $this->assertSame('public-bytes', $disk->get(dirname($target_public) . '/DELETED_deck.pptx')); } public function testFailedDoesNotDeleteReplacedUpload(): void @@ -348,7 +414,7 @@ public function testFailedDoesNotDeleteReplacedUpload(): void self::$em->flush(); $target_id = $target->getId(); - (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + ($this->makeJob($source->getId(), $target_id))->failed(new \RuntimeException('boom')); $this->assertNotNull($this->findUpload($target_id)); } @@ -361,7 +427,7 @@ public function testFailedDoesNotDeleteSameFilenameReupload(): void 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->makeJob($source->getId(), $target_id))->failed(new \RuntimeException('boom')); $this->assertNotNull($this->findUpload($target_id)); $this->assertSame('staff-bytes', Storage::disk('local')->get($target_private)); @@ -374,7 +440,7 @@ public function testHandleThrowsWhenTargetStorageLocked(): void $this->withStorageLockHeld($target, function () use ($source, $target) { try { - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $this->fail('expected UnacquiredLockException'); } catch (UnacquiredLockException $ex) { $this->assertFalse(Storage::disk('local')->exists($target->getRelativePath(IStorageTypesConstants::PrivateType))); @@ -396,7 +462,7 @@ public function testHandleThrowsWhenSameLocationLockedByDifferentUpload(): void $name = SummitService::mediaUploadStorageLockName(self::$clonePresentation->getId(), 'deck.pptx'); $token = $locks->acquireLock($name, 60); try { - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $this->fail('expected UnacquiredLockException'); } catch (UnacquiredLockException $ex) { $disk = Storage::disk('local'); @@ -414,7 +480,7 @@ public function testFailedKeepsUploadWhenTargetStorageLocked(): void Log::spy(); $this->withStorageLockHeld($target, function () use ($source, $target_id) { - (new CopyClonedMediaUploadFilesJob($source->getId(), $target_id, 'deck.pptx'))->failed(new \RuntimeException('boom')); + ($this->makeJob($source->getId(), $target_id))->failed(new \RuntimeException('boom')); }); $this->assertNotNull($this->findUpload($target_id)); @@ -445,10 +511,10 @@ public function testFailedAfterTimeoutRemovesCloneUploadUnderItsOwnLock(): void $held = new \ReflectionProperty(SummitService::class, 'held_media_upload_lock_tokens'); $held->setAccessible(true); - $held->setValue(null, [$lock_name => $token]); + $held->setValue(null, [$lock_name => [[$lock_name, $token]]]); try { - (new CopyClonedMediaUploadFilesJob($source_id, $target_id, 'deck.pptx')) + ($this->makeJob($source_id, $target_id)) ->failed(new \Illuminate\Queue\TimeoutExceededException('timed out')); } finally { $held->setValue(null, []); @@ -472,7 +538,7 @@ public function testHandleThrowsWhenSourceReuploadInFlight(): void $this->markReuploaded($source, PendingMediaUpload::STATUS_PROCESSING); try { - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $this->fail('expected RuntimeException'); } catch (\RuntimeException $ex) { $this->assertStringContainsString('in flight', $ex->getMessage()); @@ -486,8 +552,74 @@ public function testHandleCopiesWhenSourceReuploadCompleted(): void $this->seedSource($source); $this->markReuploaded($source, PendingMediaUpload::STATUS_COMPLETED); - $this->runJob(new CopyClonedMediaUploadFilesJob($source->getId(), $target->getId(), 'deck.pptx')); + $this->runJob($this->makeJob($source->getId(), $target->getId())); $this->assertSame('private-bytes', Storage::disk('local')->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); } + + /** + * The original's location is locked while its processor writes it, and while another copy + * fills it when the original is itself a clone. Reading it then could copy a missing or + * half-written leg, so the copy waits instead and gives the target's lock back. + */ + public function testHandleThrowsWhenSourceStorageLocked(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + + $this->withStorageLockHeld($source, function () use ($source, $target) { + try { + $this->runJob($this->makeJob($source->getId(), $target->getId())); + $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))); + } + }); + + // the target's lock, taken first, was released when the source's could not be taken + $locks = App::make(ILockManagerService::class); + $target_lock = SummitService::mediaUploadStorageLockName(self::$clonePresentation->getId(), 'deck.pptx'); + $locks->releaseLock($target_lock, $locks->acquireLock($target_lock, 60)); + } + + public function testHandleReleasesBothLocksAfterCopy(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + + $this->runJob($this->makeJob($source->getId(), $target->getId())); + + $locks = App::make(ILockManagerService::class); + foreach ([self::$original, self::$clonePresentation] as $presentation) { + $name = SummitService::mediaUploadStorageLockName($presentation->getId(), 'deck.pptx'); + // acquireLock throws when the copy left the name locked + $token = $locks->acquireLock($name, 60); + $this->assertNotEmpty($token); + $locks->releaseLock($name, $token); + } + } + + /** + * A re-upload of the original under a new name leaves the cloned file in storage beside the new + * one. The clone's record still says deck.pptx, so it must get deck.pptx's bytes. + */ + public function testHandleCopiesTheClonedFileAfterSourceReuploadedUnderNewName(): void + { + [$source, $target] = $this->makePair(); + $this->seedSource($source); + $source->setFilename('final.pdf'); + self::$em->flush(); + $disk = Storage::disk('local'); + $disk->put($source->getRelativePath(IStorageTypesConstants::PrivateType), 'new-private-bytes'); + $disk->put($source->getRelativePath(IStorageTypesConstants::PublicType), 'new-public-bytes'); + $this->markReuploaded($source, PendingMediaUpload::STATUS_COMPLETED); + + $this->runJob($this->makeJob($source->getId(), $target->getId())); + + $this->assertSame('deck.pptx', $target->getFilename()); + $this->assertSame('private-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PrivateType))); + $this->assertSame('public-bytes', $disk->get($target->getRelativePath(IStorageTypesConstants::PublicType))); + } } diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php index 75bd5900cd..5276915e8d 100644 --- a/tests/SummitServiceTest.php +++ b/tests/SummitServiceTest.php @@ -479,9 +479,11 @@ public function testCloneEventQueuesCopyAndCopyLandsFiles(): void ); Bus::assertDispatchedTimes(CopyClonedMediaUploadFilesJob::class, 1); - Bus::assertDispatched(CopyClonedMediaUploadFilesJob::class, function (CopyClonedMediaUploadFilesJob $job) use ($upload, $clone_upload) { + Bus::assertDispatched(CopyClonedMediaUploadFilesJob::class, function (CopyClonedMediaUploadFilesJob $job) use ($presentation, $upload, $clone, $clone_upload) { return $job->source_media_upload_id === $upload->getId() + && $job->source_presentation_id === $presentation->getId() && $job->target_media_upload_id === $clone_upload->getId() + && $job->target_presentation_id === $clone->getId() && $job->filename === 'deck.pptx'; }); diff --git a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php index f48accc5cc..c271639993 100644 --- a/tests/Unit/Services/ProcessPendingMediaUploadsTest.php +++ b/tests/Unit/Services/ProcessPendingMediaUploadsTest.php @@ -15,6 +15,7 @@ use App\Models\Foundation\Summit\Repositories\IPendingMediaUploadRepository; use App\Services\Utils\Exceptions\UnacquiredLockException; use App\Services\Utils\ILockManagerService; +use libs\utils\ICacheService; use Illuminate\Container\Container; use Illuminate\Support\Facades\Facade; use services\model\ISummitService; @@ -77,7 +78,8 @@ private function createServiceWithDeps( IPendingMediaUploadRepository $pendingRepo, ITransactionService $txService, ?ISummitRepository $summitRepository = null, - ?ILockManagerService $lockService = null + ?ILockManagerService $lockService = null, + ?ICacheService $cacheService = null ): SummitService { $ref = new \ReflectionClass(SummitService::class); $service = $ref->newInstanceWithoutConstructor(); @@ -103,6 +105,15 @@ private function createServiceWithDeps( $prop->setAccessible(true); $prop->setValue($service, $lockService); + // Default: a reachable lock store, so a lock miss means the lock is really held. + if (!$cacheService) { + $cacheService = Mockery::mock(ICacheService::class); + $cacheService->shouldReceive('exists')->andReturn(true); + } + $prop = $ref->getProperty('cache_service'); + $prop->setAccessible(true); + $prop->setValue($service, $cacheService); + // tx_service is protected in AbstractService (grandparent of SummitService) $parentRef = $ref; while ($parentRef && !$parentRef->hasProperty('tx_service')) { @@ -402,7 +413,7 @@ public function testLockedUploadIsSkippedWithoutTouchingIt(): void $lockService = Mockery::mock(ILockManagerService::class); $lockService->shouldReceive('acquireLock')->once() - ->with(SummitService::mediaUploadStorageLockName(103, 'deck.pptx'), ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME) + ->with(SummitService::mediaUploadStorageLockName(103, 'deck.pptx'), ISummitService::PENDING_MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME) ->andThrow(new UnacquiredLockException('upload 1 storage is locked')); $lockService->shouldNotReceive('releaseLock'); @@ -423,6 +434,102 @@ public function testLockedUploadIsSkippedWithoutTouchingIt(): void $this->assertEquals(0, $stats['errors']); } + /** + * The lock store being down must not stall staff uploads: a lock miss with no key behind it is + * an unreachable store, not a holder, so the row is processed without the lock. + * @dataProvider lockStoreFailureProvider + */ + public function testUploadIsProcessedWhenLockStoreIsUnavailable(\Throwable $acquire_failure): void + { + $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); + $txService = Mockery::mock(ITransactionService::class); + $summitRepository = Mockery::mock(ISummitRepository::class); + + $pendingUpload = Mockery::mock(PendingMediaUpload::class); + $pendingUpload->shouldReceive('getId')->andReturn(1); + $pendingUpload->shouldReceive('getMediaUpload')->andReturn($this->mediaUploadMock(104)); + $pendingUpload->shouldReceive('getFileName')->andReturn('deck.pptx'); + $pendingUpload->shouldReceive('getAttempts')->andReturnValues([0, 1]); + // the row IS attempted: marked Processing, then fails on the missing summit + $pendingUpload->shouldReceive('setStatus')->with(PendingMediaUpload::STATUS_PROCESSING)->once(); + $pendingUpload->shouldReceive('incrementAttempts')->once(); + $pendingUpload->shouldReceive('getSummitId')->andReturn(999); + $summitRepository->shouldReceive('getById')->with(999)->andReturn(null); + $pendingUpload->shouldReceive('setErrorMessage')->once()->with(Mockery::pattern('/Summit 999 not found/')); + + $lockService = Mockery::mock(ILockManagerService::class); + $lockService->shouldReceive('acquireLock')->once()->andThrow($acquire_failure); + $lockService->shouldNotReceive('releaseLock'); + $cacheService = Mockery::mock(ICacheService::class); + $cacheService->shouldReceive('exists')->andReturn(false); + + $pendingRepo->shouldReceive('resetStuckProcessingRows')->once()->andReturn(0); + $pendingRepo->shouldReceive('getPendingUploads')->once()->andReturn([$pendingUpload]); + // the rest of the run still happens: a lock store error never aborts the batch + $pendingRepo->shouldReceive('deleteCompletedOlderThan')->once()->with(7, 1000)->andReturn(0); + + $txService->shouldReceive('transaction')->andReturnUsing(function ($callback) { + return $callback(); + }); + + $service = $this->createServiceWithDeps($pendingRepo, $txService, $summitRepository, $lockService, $cacheService); + + $stats = $service->processPendingMediaUploads(3); + + $this->assertEquals(1, $stats['errors']); + } + + public static function lockStoreFailureProvider(): array + { + return [ + 'store unreachable (reported as a lock miss)' => [new UnacquiredLockException('lock name x')], + 'store error (e.g. read-only replica)' => [new \RuntimeException('READONLY You can\'t write against a read only replica.')], + ]; + } + + /** + * A lock that cannot be released expires on its own; the rows after it are still processed. + */ + public function testLockReleaseFailureDoesNotAbortTheBatch(): void + { + $pendingRepo = Mockery::mock(IPendingMediaUploadRepository::class); + $txService = Mockery::mock(ITransactionService::class); + $summitRepository = Mockery::mock(ISummitRepository::class); + $summitRepository->shouldReceive('getById')->with(999)->andReturn(null); + + $rows = []; + foreach ([1, 2] as $id) { + $row = Mockery::mock(PendingMediaUpload::class); + $row->shouldReceive('getId')->andReturn($id); + $row->shouldReceive('getMediaUpload')->andReturn($this->mediaUploadMock(200 + $id)); + $row->shouldReceive('getFileName')->andReturn('deck.pptx'); + $row->shouldReceive('getAttempts')->andReturnValues([0, 1]); + $row->shouldReceive('setStatus')->with(PendingMediaUpload::STATUS_PROCESSING)->once(); + $row->shouldReceive('incrementAttempts')->once(); + $row->shouldReceive('getSummitId')->andReturn(999); + $row->shouldReceive('setErrorMessage')->once(); + $rows[] = $row; + } + + $lockService = Mockery::mock(ILockManagerService::class); + $lockService->shouldReceive('acquireLock')->twice()->andReturn('test-token'); + $lockService->shouldReceive('releaseLock')->twice()->andThrow(new \RuntimeException('connection lost')); + + $pendingRepo->shouldReceive('resetStuckProcessingRows')->once()->andReturn(0); + $pendingRepo->shouldReceive('getPendingUploads')->once()->andReturn($rows); + $pendingRepo->shouldReceive('deleteCompletedOlderThan')->once()->with(7, 1000)->andReturn(0); + + $txService->shouldReceive('transaction')->andReturnUsing(function ($callback) { + return $callback(); + }); + + $service = $this->createServiceWithDeps($pendingRepo, $txService, $summitRepository, $lockService); + + $stats = $service->processPendingMediaUploads(3); + + $this->assertEquals(2, $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.