Repository navigation
fix(media-uploads): copy media upload files when cloning an activity; freeze modern-format upload paths - #643
caseylocker wants to merge 14 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.
 |
|
📘 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); |
There was a problem hiding this comment.
Fixed in 0b9476c. The copy now takes the source location's storage lock as well as the target's, and holds both for the whole copy, so the source's processor (or another copy still filling it, when the source is itself a clone) cannot write the files while they are read. If the source lock cannot be taken, the target lock is released and the job retries.
No lock ordering is needed: acquireLock never blocks (three tries, then UnacquiredLockException), so two copies taking the locks in opposite order cannot deadlock. The job's backoff now adds up to more than the lock lifetime, so a waiting copy outlasts a held lock.
Covered by testHandleThrowsWhenSourceStorageLocked and testHandleReleasesBothLocksAfterCopy.
| 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); |
There was a problem hiding this comment.
Fixed in 0b9476c, without a separate EntityManager.
- The job payload now carries both presentation IDs, so the lock names are built and the locks taken before anything is read from the database.
- Under the locks, target and source are read with a new
getByIdFresh(), a DQL read withQuery::HINT_REFRESH. It does not answer from the identity map and returns null when the row is gone, so a delete then re-add at the same path is seen. - The
failed()cleanup uses the same read withFOR UPDATE, so it no longer renames a file that a newer upload wrote at that path.
Covered by testHandleLeavesFileAloneWhenTargetDeletedByAnotherProcess and testFailedLeavesFileAloneWhenTargetDeletedByAnotherProcess, which delete the row outside the EntityManager so the stale entity stays managed.
… 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
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-643/ This page is automatically updated on each push to this PR. |
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 (8 tries over about 23 minutes, timeout 1200 s, belowretry_after). It copies the filename the clone was queued with, so a later re-upload of the original under a new name does not change what the clone gets.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. The job holds the lock of both the clone's location and the original's, so a staff upload landing mid-copy is written after the copy, never overwritten by it, and an original is never read while it is being written. The processor skips a locked row untouched and picks it up on the next minute's run. If the lock store itself is unreachable, the processor keeps processing without the lock; a copy waits and retries instead.Tests
CopyClonedMediaUploadFilesJobTest(26),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; lock store unavailable; release failure).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. Checked against prod on Nov 13: 221 rows are in scope and the longest title is 113 characters, well inside the 1024-character column.LockManagerService ... MAX RETRIESERROR lines while a clone copy holds a lock the processor wants, or while one copy waits on another (for example a clone of a clone). They are lock misses, not failures: the processor picks the row up on its next run and the copy retries.CopyClonedMediaUploadFilesJob::failedin the log is the signal that staff need to re-upload on the clone.lock store unavailable, processing without the storage lock(WARNING) for each row and keeps writing staff uploads. Clone copies do not run without the lock. If the outage outlasts a copy's retries, its cleanup cannot take the lock either: it keeps the clone's upload and logscould not take the storage lock to remove clone media upload(ERROR), and that upload needs a delete or re-upload on the clone.Known limits