From 954a335f3106242b8b84700a4bfbaa59a4a8b8ed Mon Sep 17 00:00:00 2001 From: 4gray Date: Wed, 14 Oct 2026 20:06:18 +0200 Subject: [PATCH] fix(server-storage): the move uploads playlists create-only and merges into a copy moved first Two tabs or devices moving browser copies with the same playlist id both planned an upload; the browser documents carry no served version, so both PUTs were unconditional and the later one overwrote the first copy (and any change made on it since), with both moves reporting success. The backend's playlist PUT now honours `If-None-Match: *` (412 when the playlist exists; checked and written in one synchronous handler). The move uploads with it through ServerPlaylistDocumentStore .dbCreateAppPlaylist, and an upload that finds its id taken merges its favorites and history into that copy instead, counted as merged. Co-Authored-By: Claude Opus 5.5 --- .../src/app/storage/playlists-router.ts | 9 ++++ .../src/app/storage/storage-routes.spec.ts | 29 +++++++++++++ docs/architecture/pwa-self-hosted.md | 6 ++- .../server-migration.service.spec.ts | 42 ++++++++++++++++--- .../server-migration.service.ts | 19 +++++++-- .../server-playlist-document-store.spec.ts | 13 ++++++ .../server-playlist-document-store.ts | 23 ++++++++++ 7 files changed, 129 insertions(+), 12 deletions(-) diff --git a/apps/web-backend/src/app/storage/playlists-router.ts b/apps/web-backend/src/app/storage/playlists-router.ts index d755fe4f2..ebf85b955 100644 --- a/apps/web-backend/src/app/storage/playlists-router.ts +++ b/apps/web-backend/src/app/storage/playlists-router.ts @@ -64,6 +64,15 @@ export function createPlaylistsRouter( 'The playlist changed since it was read.' ); } + // A create-only upload (a move from browser storage) that another + // tab or device made first. Checked and written in one handler. + if (req.get('if-none-match')?.trim() === '*' && playlists.exists(id)) { + throw new StorageRequestError( + 412, + 'conflict', + 'A playlist with this id exists already.' + ); + } playlists.upsert( { ...requireDocument(req.body), _id: id }, { keepPayload: req.query['keepPayload'] === '1' } diff --git a/apps/web-backend/src/app/storage/storage-routes.spec.ts b/apps/web-backend/src/app/storage/storage-routes.spec.ts index 9b915b154..5059c680e 100644 --- a/apps/web-backend/src/app/storage/storage-routes.spec.ts +++ b/apps/web-backend/src/app/storage/storage-routes.spec.ts @@ -251,6 +251,35 @@ describe('server storage routes', () => { }) )); + it('creates a playlist create-only, refused once it exists', () => + withTempStorage((storage) => + withServer(app(storage), async (baseUrl) => { + const create = (title: string) => + fetch( + `${baseUrl}/v1/playlists/p1`, + json( + { ...m3uDocument('p1'), title }, + { + method: 'PUT', + headers: { 'if-none-match': '*' }, + } + ) + ); + + expect((await create('First')).status).toBe(200); + // Another tab or device moved the same playlist first. + const second = await create('Second'); + expect(second.status).toBe(412); + await expect(second.json()).resolves.toMatchObject({ + code: 'conflict', + }); + const stored = await fetch(`${baseUrl}/v1/playlists/p1`); + await expect(stored.json()).resolves.toMatchObject({ + title: 'First', + }); + }) + )); + it('writes user state only on the tag it was read at', () => withTempStorage((storage) => withServer(app(storage), async (baseUrl) => { diff --git a/docs/architecture/pwa-self-hosted.md b/docs/architecture/pwa-self-hosted.md index 9488624c8..c3e7fab5b 100644 --- a/docs/architecture/pwa-self-hosted.md +++ b/docs/architecture/pwa-self-hosted.md @@ -86,7 +86,8 @@ Server storage adds versioned routes under `/v1`, which the PWA uses in `header` fields (`409 payload_missing` without a stored row); every `PUT` answers with the new `ETag` and `X-Replaced-ETag`, the version it replaced, and one with `If-Match` answers `412 conflict` when another - write replaced that version + write replaced that version, one with `If-None-Match: *` when the + playlist exists already - `GET /v1/user-state`, `PUT|DELETE /v1/user-state/:namespace/:playlistId`: Xtream favorites, recently viewed, collection snapshots and playback positions, one JSON value per namespace and playlist; `GET` adds a @@ -363,7 +364,8 @@ settings and guards read them in every build. a conditional mutation, a conflict retried at the next start). A playlist the server already has, by id or by source (Xtream server and user, Stalker portal and MAC, M3U URL), merges into the server's copy; the - others upload with their ids. A merged playlist gains the browser + others upload with their ids, create-only (`If-None-Match: *`): one that + another tab or device moved first is merged into instead of overwritten. A merged playlist gains the browser document's favorites and history (M3U favorites name channels by stream URL, as the app looks them up; older ones by a channel id that is random per import, which becomes its URL on both sides; history entries keep diff --git a/libs/services/src/lib/server-storage/server-migration.service.spec.ts b/libs/services/src/lib/server-storage/server-migration.service.spec.ts index 472b3d952..3bec5bcff 100644 --- a/libs/services/src/lib/server-storage/server-migration.service.spec.ts +++ b/libs/services/src/lib/server-storage/server-migration.service.spec.ts @@ -22,6 +22,7 @@ describe('ServerMigrationService', () => { dbGetAppPlaylist: jest.Mock; dbGetAppPlaylistMetas: jest.Mock; dbUpsertAppPlaylist: jest.Mock; + dbCreateAppPlaylist: jest.Mock; }; api: { getJson: jest.Mock; sendJson: jest.Mock }; userData: { @@ -52,6 +53,7 @@ describe('ServerMigrationService', () => { dbUpsertAppPlaylist: jest .fn() .mockResolvedValue({ success: true }), + dbCreateAppPlaylist: jest.fn().mockResolvedValue(true), }, api: { getJson: jest.fn().mockResolvedValue({ entries: {} }), @@ -117,7 +119,7 @@ describe('ServerMigrationService', () => { const result = await migration.moveBrowserData(); expect(result).toEqual({ imported: 2, merged: 0, failed: 0 }); - expect(session.documentStore.dbUpsertAppPlaylist).toHaveBeenCalledTimes( + expect(session.documentStore.dbCreateAppPlaylist).toHaveBeenCalledTimes( 2 ); expect(session.api.sendJson).toHaveBeenCalledWith( @@ -353,10 +355,38 @@ describe('ServerMigrationService', () => { }); }); + it('merges into a copy another tab or device uploaded first', async () => { + TestBed.overrideProvider(NgxIndexedDBService, { + useValue: { + getAll: () => + of([{ ...browserPlaylists[0], favorites: ['http://s/2'] }]), + }, + }); + // The id was free when planned, taken when uploaded. + session.documentStore.dbCreateAppPlaylist.mockResolvedValue(false); + const first = { + ...browserPlaylists[0], + favorites: ['http://s/1'], + recentlyViewed: [], + }; + session.documentStore.dbGetAppPlaylist.mockResolvedValue(first); + + const result = await TestBed.inject( + ServerMigrationService + ).moveBrowserData(); + + expect(result).toEqual({ imported: 0, merged: 1, failed: 0 }); + // Never overwritten: this browser's favorites join the first copy's. + expect(session.documentStore.dbUpsertAppPlaylist).toHaveBeenCalledWith({ + ...first, + favorites: ['http://s/1', 'http://s/2'], + }); + }); + it('keeps the move on offer when a playlist could not be uploaded', async () => { - session.documentStore.dbUpsertAppPlaylist + session.documentStore.dbCreateAppPlaylist .mockRejectedValueOnce(new Error('413')) - .mockResolvedValue({ success: true }); + .mockResolvedValue(true); const migration = TestBed.inject(ServerMigrationService); const result = await migration.moveBrowserData(); @@ -382,7 +412,7 @@ describe('ServerMigrationService', () => { await expect(migration.moveBrowserData()).rejects.toThrow('Unsent'); expect( - session.documentStore.dbUpsertAppPlaylist + session.documentStore.dbCreateAppPlaylist ).not.toHaveBeenCalled(); expect(session.api.sendJson).not.toHaveBeenCalled(); expect(migration.browserDataMoved()).toBe(false); @@ -398,7 +428,7 @@ describe('ServerMigrationService', () => { ]), }, }); - session.documentStore.dbUpsertAppPlaylist.mockRejectedValue( + session.documentStore.dbCreateAppPlaylist.mockRejectedValue( new Error('500') ); localStorage.setItem( @@ -501,7 +531,7 @@ describe('ServerMigrationService', () => { await TestBed.inject(ServerMigrationService).moveBrowserData(); - expect(session.documentStore.dbUpsertAppPlaylist).toHaveBeenCalledWith( + expect(session.documentStore.dbCreateAppPlaylist).toHaveBeenCalledWith( expect.objectContaining({ _id: 's1', isFullStalkerPortal: true }) ); }); diff --git a/libs/services/src/lib/server-storage/server-migration.service.ts b/libs/services/src/lib/server-storage/server-migration.service.ts index f82d16383..e6e25438f 100644 --- a/libs/services/src/lib/server-storage/server-migration.service.ts +++ b/libs/services/src/lib/server-storage/server-migration.service.ts @@ -130,10 +130,21 @@ export class ServerMigrationService { merged += 1; targets.set(step.browserId, step.serverId); } else { - await this.session.documentStore.dbUpsertAppPlaylist( - step.playlist - ); - imported += 1; + // Create-only: another tab or device that moved the + // same playlist first keeps its copy, merged into. + const created = + await this.session.documentStore.dbCreateAppPlaylist( + step.playlist + ); + if (created) { + imported += 1; + } else { + await this.mergeDocumentUserData( + step.playlist, + step.playlist._id + ); + merged += 1; + } onServer.add(step.playlist._id); targets.set(step.playlist._id, step.playlist._id); } diff --git a/libs/services/src/lib/server-storage/server-playlist-document-store.spec.ts b/libs/services/src/lib/server-storage/server-playlist-document-store.spec.ts index ae5188c43..3a71fe0d7 100644 --- a/libs/services/src/lib/server-storage/server-playlist-document-store.spec.ts +++ b/libs/services/src/lib/server-storage/server-playlist-document-store.spec.ts @@ -170,6 +170,19 @@ describe('ServerPlaylistDocumentStore', () => { expect(api.sendJson.mock.calls[1][3]).toEqual({}); }); + it('creates a playlist only while the server has none of that id', async () => { + const { api, store } = setup(); + api.sendJson.mockResolvedValueOnce(putResponse('"b.0.1"', '')); + + await expect(store.dbCreateAppPlaylist(doc)).resolves.toBe(true); + expect(api.sendJson.mock.calls[0][3]).toEqual({ 'If-None-Match': '*' }); + + api.sendJson.mockRejectedValueOnce( + new ServerApiError(412, 'conflict', 'exists') + ); + await expect(store.dbCreateAppPlaylist(doc)).resolves.toBe(false); + }); + it('writes a portal document on the version it read', async () => { const { api, store } = setup(); const portal = { _id: 'p2', title: 'Portal', macAddress: 'm' }; diff --git a/libs/services/src/lib/server-storage/server-playlist-document-store.ts b/libs/services/src/lib/server-storage/server-playlist-document-store.ts index 7e41995b4..4e9bb7228 100644 --- a/libs/services/src/lib/server-storage/server-playlist-document-store.ts +++ b/libs/services/src/lib/server-storage/server-playlist-document-store.ts @@ -191,6 +191,29 @@ export class ServerPlaylistDocumentStore implements PlaylistDocumentStore { } } + /** + * A create-only upload (the move from browser storage): false when the + * server has a playlist with that id already, uploaded meanwhile by + * another tab or device, which the caller merges into instead. + */ + async dbCreateAppPlaylist(playlist: Playlist): Promise { + try { + const response = await this.api.sendJson( + 'PUT', + `/playlists/${encodeURIComponent(playlist._id)}`, + playlist, + { 'If-None-Match': '*' } + ); + this.remember(playlist._id, response, playlist); + return true; + } catch (error) { + if (error instanceof ServerApiError && error.status === 412) { + return false; + } + throw error; + } + } + /** `null` when the server has no payload to keep (write it in full). */ private async keepPayload( playlist: Playlist,