From cb858edd34446945d8df754bb71c9a5e0d54f5ea Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Thu, 3 Sep 2026 20:43:21 +0200 Subject: [PATCH] Serialize playlist mutations and re-resolve on credential change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes to a server-side playlist could overlap: the second planned its write from the snapshot the first had already invalidated, and on Piped — where a row is a position — its preflight then passed over whatever had shifted into the slot. Mutations now hold a per-playlist queue from the write to the end of the refetch that reconciles it. Creation queues on the account's list instead: it depends on no playlist's contents, and one queue per account would only make unrelated changes wait. The playlist detail view kept the authenticated snapshot in its own state, so signing in as somebody else — or a change of Apple account — left the previous login's rows, title and destructive affordances on screen. It now discards them the moment either lands and loads again, which re-resolves ownership and falls back to the public endpoint. Also documents two residuals rather than pretending to close them: equal rows are indistinguishable to a positional API, so verify-before-remove cannot protect a particular occurrence of a repeated video; and the credential recheck is a fence, not a lock, because Keychain state cannot be made atomic with a remote send. --- Yattee/Services/AccountService.swift | 20 +++ Yattee/Services/RemotePlaylistsCache.swift | 134 ++++++++++++++---- .../Playlist/UnifiedPlaylistDetailView.swift | 42 ++++++ YatteeTests/AccountServiceTests.swift | 71 ++++++++++ 4 files changed, 236 insertions(+), 31 deletions(-) diff --git a/Yattee/Services/AccountService.swift b/Yattee/Services/AccountService.swift index 71191a09..37c96ce3 100644 --- a/Yattee/Services/AccountService.swift +++ b/Yattee/Services/AccountService.swift @@ -274,6 +274,16 @@ final class AccountService { /// refetch afterwards can show what actually happened. Invidious needs none /// of this: its `indexId` names the row itself, whatever moved around it. /// + /// Two rows of the same video are one row to a positional API, so the check + /// is weaker still where a playlist repeats one: it can say the position + /// holds that video, never that it holds *this* occurrence of it. In + /// `[A, A, B]`, another client removing the first `A` leaves the second in + /// its place and the check passes over the survivor. Nothing Piped exposes + /// tells two equal rows apart — there is no token for the row, only its + /// index — so no stronger comparison exists to make here; a fingerprint of + /// the neighbours would refuse legitimate removals without ever settling + /// the duplicate case. The refetch afterwards is what shows the result. + /// /// The preflight is also where the credential can change out from under the /// removal — it is the one place here that awaits a server before writing — /// so the credential is read again before the removal is sent. @@ -348,6 +358,16 @@ final class AccountService { /// Piped's removals in particular await a whole playlist fetch first. A /// stale *read* is merely wasted; a stale *write* changes the previous /// user's data, so every mutation checks again immediately before it goes. + /// + /// It is a fence, not a lock, and the last of it is accepted rather than + /// closed: between this check and the bytes leaving the device there is at + /// least an actor hop, and a sign-out landing inside it still sends the old + /// secret. Keychain state cannot be made atomic with a remote send — + /// cancelling in-flight work or handing the transport a generation would + /// move the window, not remove it — so the recheck is the practical fence, + /// and what follows the send is what covers the remainder: an outcome is + /// recorded only against the secret the request actually used, so a late + /// answer never lands on a credential that did not earn it. private func requireStillCurrent(_ credential: String, for account: Account) throws { guard credentialReader(account) == credential else { throw AccountServiceError.notAuthenticated diff --git a/Yattee/Services/RemotePlaylistsCache.swift b/Yattee/Services/RemotePlaylistsCache.swift index 6b645f98..d485061d 100644 --- a/Yattee/Services/RemotePlaylistsCache.swift +++ b/Yattee/Services/RemotePlaylistsCache.swift @@ -74,6 +74,30 @@ final class RemotePlaylistsCache { let playlistID: String } + /// What a mutation holds while it runs, so two of them cannot overlap. + /// + /// A change and the refetch that reconciles it are one operation, and the + /// next change to the same playlist may not start until it is over: it + /// plans its write against the last snapshot, and a snapshot taken before + /// the previous change landed describes a playlist that no longer exists. + /// Piped is where this bites — its rows are addressed by *position*, so a + /// second removal that preflights the pre-removal snapshot passes its check + /// and then deletes whatever shifted into the slot. + /// + /// Creation queues on the account's *list* instead, and deliberately not + /// with the playlists: nothing it does depends on any playlist's contents, + /// and the list refetches that two mutations race over are already ordered + /// by `sequence`. One queue for the account would only make changes to + /// unrelated playlists wait for each other. + private enum MutationKey: Hashable { + case list(accountID: UUID) + case playlist(accountID: UUID, playlistID: String) + } + + /// Who is queued behind each held key, in the order they arrived. A key + /// present is held; absent is free. + private var mutationWaiters: [MutationKey: [CheckedContinuation]] = [:] + // MARK: - Reads /// The account's playlists as last fetched, or `nil` if they never were. @@ -125,13 +149,15 @@ final class RemotePlaylistsCache { privacy: PlaylistPrivacy?, for account: Account ) async throws -> Playlist { - try await reconcilingList(for: account) { - try await accountService.createPlaylist( - title: title, - description: description, - privacy: privacy, - for: account - ) + try await serializing(.list(accountID: account.id)) { + try await reconcilingList(for: account) { + try await accountService.createPlaylist( + title: title, + description: description, + privacy: privacy, + for: account + ) + } } } @@ -143,38 +169,45 @@ final class RemotePlaylistsCache { privacy: PlaylistPrivacy?, for account: Account ) async throws { - try await reconciling(playlist: id, andList: true, for: account) { - try await accountService.updatePlaylist( - id: id, - title: title, - description: description, - privacy: privacy, - for: account - ) + try await serializing(.playlist(accountID: account.id, playlistID: id)) { + try await reconciling(playlist: id, andList: true, for: account) { + try await accountService.updatePlaylist( + id: id, + title: title, + description: description, + privacy: privacy, + for: account + ) + } } } /// Deletes a playlist and republishes the account's list. func deletePlaylist(id: String, for account: Account) async throws { - do { - try await accountService.deletePlaylist(id: id, for: account) - forgetPlaylist(id: id, for: account.id) - await reconcileList(for: account) - } catch { - // The delete may have landed even though the answer did not, so the - // snapshot is refetched rather than assumed: whichever way it went, - // the server's answer replaces it — and a server that no longer has - // the playlist is answering that the delete did land. - await reconcileSnapshotAfterDelete(id: id, for: account) - await reconcileList(for: account) - throw error + try await serializing(.playlist(accountID: account.id, playlistID: id)) { + do { + try await accountService.deletePlaylist(id: id, for: account) + forgetPlaylist(id: id, for: account.id) + await reconcileList(for: account) + } catch { + // The delete may have landed even though the answer did not, so + // the snapshot is refetched rather than assumed: whichever way + // it went, the server's answer replaces it — and a server that + // no longer has the playlist is answering that the delete did + // land. + await reconcileSnapshotAfterDelete(id: id, for: account) + await reconcileList(for: account) + throw error + } } } /// Adds a video and republishes the playlist it was added to. func addVideo(videoID: String, toPlaylist id: String, for account: Account) async throws { - try await reconciling(playlist: id, andList: false, for: account) { - try await accountService.addVideo(videoID: videoID, toPlaylist: id, for: account) + try await serializing(.playlist(accountID: account.id, playlistID: id)) { + try await reconciling(playlist: id, andList: false, for: account) { + try await accountService.addVideo(videoID: videoID, toPlaylist: id, for: account) + } } } @@ -184,8 +217,10 @@ final class RemotePlaylistsCache { fromPlaylist id: String, for account: Account ) async throws { - try await reconciling(playlist: id, andList: false, for: account) { - try await accountService.removeEntry(entry, fromPlaylist: id, for: account) + try await serializing(.playlist(accountID: account.id, playlistID: id)) { + try await reconciling(playlist: id, andList: false, for: account) { + try await accountService.removeEntry(entry, fromPlaylist: id, for: account) + } } } @@ -383,6 +418,43 @@ final class RemotePlaylistsCache { lists[accountID]?.removeAll { $0.id.playlistID == id } } + // MARK: - Serialization + + /// Runs a mutation with its key held, from the write to the end of the + /// refetch that reconciles it, and hands the key to whoever queued behind. + /// + /// First come, first served: waiters are resumed in arrival order, so two + /// removals tapped in sequence are sent in that sequence. Nothing here + /// blocks a *read* — refreshes stay coalesced and unqueued, and only a + /// change waits for a change. + private func serializing(_ key: MutationKey, _ body: () async throws -> T) async throws -> T { + await acquire(key) + defer { release(key) } + return try await body() + } + + private func acquire(_ key: MutationKey) async { + guard mutationWaiters[key] != nil else { + mutationWaiters[key] = [] + return + } + + await withCheckedContinuation { continuation in + mutationWaiters[key, default: []].append(continuation) + } + } + + private func release(_ key: MutationKey) { + guard var waiting = mutationWaiters[key], !waiting.isEmpty else { + mutationWaiters[key] = nil + return + } + + let next = waiting.removeFirst() + mutationWaiters[key] = waiting + next.resume() + } + // MARK: - Fences private func isCurrent(_ generation: Int, for accountID: UUID) -> Bool { diff --git a/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift b/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift index 7719d176..c45ecefd 100644 --- a/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift +++ b/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift @@ -287,6 +287,17 @@ struct UnifiedPlaylistDetailView: View { .onReceive(NotificationCenter.default.publisher(for: .watchHistoryDidChange)) { _ in loadWatchEntries() } + // Signing in again keeps the account, the source and usually the status, + // so nothing else this view watches would move — but the login behind an + // account-loaded playlist may now be somebody else. + .onReceive(NotificationCenter.default.publisher(for: .accountCredentialDidChange)) { _ in + discardAccountLoadedContentAndReload() + } + // The same, for a change of Apple account: every credential on the + // device belongs to an identity, and this one's are no longer readable. + .onReceive(NotificationCenter.default.publisher(for: .credentialIdentityScopeDidChange)) { _ in + discardAccountLoadedContentAndReload() + } #if !os(tvOS) .batchDownload(coordinator: downloadCoordinator) .onAppear { @@ -823,6 +834,37 @@ struct UnifiedPlaylistDetailView: View { loadWatchEntries() } + /// Throws away everything an account fetched here, and loads again. + /// + /// Cleared first and synchronously, because the reload awaits a server: + /// until it answers, a playlist that may be private would otherwise still + /// be on screen — with its rows, its title and its destructive affordances + /// — under a credential that no longer belongs to the person who fetched + /// it. The reload resolves ownership again from scratch, and falls back to + /// the public endpoint when the account can no longer read the playlist. + /// + /// Only for a playlist an account loaded: a public one shows nothing that + /// a login could make private, and reloading it would be a flash for + /// nothing. + private func discardAccountLoadedContentAndReload() { + guard case .remote = source, entries != nil || owningAccount != nil else { return } + + entries = nil + entriesByRow = [:] + owningAccount = nil + remoteIsListed = nil + remotePlaylist = nil + title = "" + descriptionText = nil + thumbnailURL = nil + videos = [] + videoCount = 0 + errorMessage = nil + isLoading = true + + Task { await loadPlaylist() } + } + private func loadWatchEntries() { watchEntriesMap = dataManager?.watchEntriesMap() ?? [:] } diff --git a/YatteeTests/AccountServiceTests.swift b/YatteeTests/AccountServiceTests.swift index 895c0fd4..857f98ec 100644 --- a/YatteeTests/AccountServiceTests.swift +++ b/YatteeTests/AccountServiceTests.swift @@ -822,4 +822,75 @@ struct RemotePlaylistsCacheTests { #expect(await harness.transport.callCount == 1) #expect(cache.playlists(for: harness.account.id)?.count == 1) } + + @Test("A second removal waits for the first one's refetch before it preflights") + func removalsOnOnePlaylistAreSerialized() async throws { + let harness = AccountServiceHarness(type: .piped) + defer { harness.tearDown() } + let cache = harness.makeCache() + let snapshot = makeSnapshot(playlistID: "pl", videoIDs: ["aaa", "bbb"], kind: .piped) + await harness.transport.setSnapshot(snapshot) + await harness.transport.holdCalls() + + let first = Task { [account = harness.account] in + try await cache.removeEntry(snapshot.entries[0], fromPlaylist: "pl", for: account) + } + try await harness.transport.waitForCalls(1) + + // Tapped while the first removal's preflight is still out. Its own + // preflight must not be sent yet: it would describe the playlist as it + // was *before* the first removal, and a Piped position taken from that + // names whatever shifts into the slot. + let second = Task { [account = harness.account] in + try await cache.removeEntry(snapshot.entries[1], fromPlaylist: "pl", for: account) + } + + // Long enough for a second preflight to have been sent, had one been. + try await Task.sleep(for: .milliseconds(50)) + #expect(await harness.transport.callCount == 1) + + await harness.transport.release() + try await first.value + try await second.value + + // Three calls each, in order, and never interleaved: preflight, remove, + // and the refetch that reconciles it. + #expect(await harness.transport.calls == [ + .snapshot(id: "pl", credential: "secret-original"), + .remove(entryID: .piped(playlistID: "pl", position: 0), credential: "secret-original"), + .snapshot(id: "pl", credential: "secret-original"), + .snapshot(id: "pl", credential: "secret-original"), + .remove(entryID: .piped(playlistID: "pl", position: 1), credential: "secret-original"), + .snapshot(id: "pl", credential: "secret-original") + ]) + } + + @Test("Removals from different playlists do not wait for each other") + func removalsOnDifferentPlaylistsInterleave() async throws { + let harness = AccountServiceHarness(type: .piped) + defer { harness.tearDown() } + let cache = harness.makeCache() + let one = makeSnapshot(playlistID: "one", videoIDs: ["aaa"], kind: .piped) + let two = makeSnapshot(playlistID: "two", videoIDs: ["aaa"], kind: .piped) + await harness.transport.setSnapshot(one) + await harness.transport.holdCalls() + + let first = Task { [account = harness.account] in + try await cache.removeEntry(one.entries[0], fromPlaylist: "one", for: account) + } + try await harness.transport.waitForCalls(1) + + let second = Task { [account = harness.account] in + try await cache.removeEntry(two.entries[0], fromPlaylist: "two", for: account) + } + + // Both preflights are out at once: the queue is per playlist, not one + // for the whole account. + try await harness.transport.waitForCalls(2) + #expect(await harness.transport.callCount == 2) + + await harness.transport.release() + try await first.value + try await second.value + } }