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 + } }