Repository navigation
fix(media-uploads): copy media upload files when cloning an activity; freeze modern-format upload paths - #643
caseylocker wants to merge 13 commits into
Conversation
… to the clone's path
…k; make the filename-guard test real
… via a per-upload lock
…torage lock dependency
… whole clone upload
…thout a cause, cover the dispatch-failure catch
…ispatched; match getPath's empty-title rule in the backfill
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-643/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-643/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved concurrency gaps can copy incomplete source files or overwrite and delete staff replacements.
2 open findings
What changed in this PR
Fixes activity cloning to copy media files and preserves modern upload paths across title or summit slug changes.
Changes:
- Adds queued file copying, shared storage locks, and failure cleanup.
- Adds and backfills frozen path segments.
- Expands regression tests and CI coverage.
| File | Description |
|---|---|
| tests/Unit/Services/ProcessPendingMediaUploadsTest.php | Tests processor lock handling. |
| tests/Unit/Migrations/FrozenPathSegmentBackfillMigrationTest.php | Tests backfill slug rules. |
| tests/Unit/Entities/PresentationMediaUploadPathTest.php | Tests stable upload paths. |
| tests/SummitServiceTest.php | Tests clone dispatch and processor locking. |
| tests/CopyClonedMediaUploadFilesJobTest.php | Tests copying, replacement guards, and cleanup. |
| database/migrations/model/Version20261014120001.php | Backfills frozen paths. |
| database/migrations/model/Version20261014120000.php | Adds the frozen-path column. |
| app/Services/Model/ISummitService.php | Defines copy and cleanup contracts. |
| app/Services/Model/Imp/SummitService.php | Implements copying, locking, and cleanup. |
| app/Repositories/Summit/DoctrinePendingMediaUploadRepository.php | Adds pending-upload existence queries. |
| app/Models/Foundation/Summit/Repositories/IPendingMediaUploadRepository.php | Declares pending-upload queries. |
| app/Models/Foundation/Summit/PendingMediaUpload.php | Centralizes in-flight statuses. |
| app/Models/Foundation/Summit/Events/Presentations/Presentation.php | Extracts reusable slug generation. |
| app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMediaUpload.php | Uses frozen modern paths. |
| app/Jobs/CopyClonedMediaUploadFilesJob.php | Adds retryable copy job. |
| .github/workflows/push.yml | Includes new tests in CI. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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); |
| 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); |

ref: https://app.clickup.com/t/9014802374/86bchyjfg
Problem
SummitService::cloneEventclonedPresentationMediaUploadrecords but not their files. The clone's path is built from the clone's presentation ID (always legacy format), so every cloned upload pointed at a path with no file. Seen in prod on OCP Global 2026 (activity 10346 cloned from 10177).getPath(), so renaming an activity (or changing the summit slug) moved the path away from the stored file. Legacy-format paths use only IDs and were unaffected.Fix
PresentationMediaUpload.FrozenPathSegment.getPath()uses it for modern-format rows; output is byte-identical for every row where it is null.Version20261014120000adds the column (Builder),Version20261014120001backfills theLegacyPathFormat = 0rows with the segmentgetPath()produces today (same empty-title rule asgenerateSlug()).cloneEventcommits, it queues oneCopyClonedMediaUploadFilesJobper cloned upload (JobDispatcher::withDbFallback). The job is a thin delegate toSummitService::copyClonedMediaUploadFiles(andremoveUncopiedClonedMediaUploadfromfailed()). The logic lives onSummitServicerather thanPresentationServicebecause it shares a storage lock withprocessPendingMediaUploads, which lives there. The copy streams each enabled storage leg from the original's path to the clone's (readStream->saveFromStream, overwriting every attempt), skips a leg the original does not have, and retries (5 tries, timeout 1200 s, belowretry_after).PendingMediaUploadrow for it), and retries while a re-upload of the original is still in flight.ILockManagerService,SummitService::mediaUploadStorageLockName=media_upload_storage.{presentation_id}.{sha1(filename)}.lock) is shared by the job andprocessPendingMediaUploads, so a staff upload landing mid-copy is written after the copy, never overwritten by it. The processor skips a locked row untouched and picks it up on the next minute's run.Tests
CopyClonedMediaUploadFilesJobTest(21),PresentationMediaUploadPathTest(7),FrozenPathSegmentBackfillMigrationTest(4).SummitServiceTest(clone queues the copy and the copy lands; dispatch failure; processor skips a locked upload),ProcessPendingMediaUploadsTest(lock dependency; locked row untouched).push.yml: adds theMigrationUnitTestsshard and the new root test file to thePresentationMediaUploadsshard.mainlocally: theOAuth2PresentationApiTest+OAuth2SummitEventsApiTestpair (17 failures, 1 error),SponsorUserPermissionTrackingTest(2), and 3 intests/Unit/Entities.Deploy notes
SELECT COUNT(*) FROM PresentationMediaUpload WHERE LegacyPathFormat = 0 AND FrozenPathSegment IS NULLshould be 0.LockManagerService ... MAX RETRIESERROR lines while a clone copy holds a lock the processor wants. They are lock misses, not failures.Known limits