From 75c1940821e48a3f8b97a1b0504718310c71e8d9 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Wed, 5 Aug 2026 16:30:12 -0500 Subject: [PATCH] Fix the worker scope that made web offline playback silently inert, and prune superseded assets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by QA against a real browser, not by any test in the suite: the bug lived entirely in the relationship between a URL and a header. A service worker's default scope is its own directory, so /player/sw.js could only ever control /player/ and below — which does not include /player itself. The player is served at all three of /player, /player/ and /player/index.html, and /player is the one that gets used: it is what the dashboard shows and what gets typed into a panel. On that URL registration SUCCEEDED, logged "Service Worker registered", and then controlled nothing. No shell cache, no content cache, no offline playback, no error. Every web and BrightSign panel served at /player has been running with its offline story switched off. Registration now asks for scope '/' and the server sends Service-Worker-Allowed to permit it. Both halves are required — without the header the registration does not narrow, it fails outright. Also: revision-keyed sweeping could not reclaim a replaced asset's predecessor. A replace writes a NEW randomly-named file, so the superseded copy lives at a different path entirely and nothing keyed on the asset path can find it; it would sit there until the quota evicted it. The player now declares the complete set of media it needs — the raw assignments, so multi-zone items are included and a prune cannot delete something a zone is still playing — and the worker drops everything else. QA results this pass: web player 18/18 against a real browser (cold start with no network renders a cached video at readyState 4); Android 12/12 on a device including a replace round-trip that re-fetched 6MB and then dropped it for the new bytes, and a cold start with the server stopped that played from disk; Tizen 11/11 for the no-storage path, which must degrade to streaming and must not claim a capability it cannot honour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- server/player/index.html | 19 ++++++-- server/player/sw.js | 24 ++++++++- server/server.js | 10 ++++ server/test/player-sw-scope.test.js | 76 +++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 server/test/player-sw-scope.test.js diff --git a/server/player/index.html b/server/player/index.html index b4c5503..9f0f80b 100644 --- a/server/player/index.html +++ b/server/player/index.html @@ -438,7 +438,11 @@ const urls = (items || []) .filter((it) => it && it.filepath && !it.remote_url) .map((it) => mediaUrl(it)); - if (urls.length) sw.postMessage({ type: 'st-cache-playlist', urls }); + // `urls` is the COMPLETE set for this display, so the worker may treat anything else in + // the content cache as superseded. That is what reclaims a replaced asset: a replace writes + // a new randomly-named file, so the old copy is not merely a different revision of the same + // URL — it is a different URL entirely, and nothing keyed on the asset path can find it. + sw.postMessage({ type: 'st-cache-playlist', urls, prune: true }); } catch (e) { /* never let caching break playback */ } } @@ -2404,7 +2408,11 @@ playlist = newItems; imgPreloadCache = {}; // playlist changed — drop stale one-ahead preloads (feat/player-image-preload) savePlaylistCache(playlist); - requestOfflineCache(playlist); + // The RAW assignments, not the split `playlist`: multi-zone items live in their own lists, + // and a keep-set that omitted them would have the worker prune assets a zone is still + // playing. This is also what makes the prune safe — it is the whole truth about what this + // display needs. + requestOfflineCache(Array.isArray(data.assignments) ? data.assignments : playlist); // #157: a fresh structural update supersedes any pending deferred rotation; the branches // below re-arm it only if the current item was removed while live in solo playback. deferredRotation = false; @@ -4044,7 +4052,12 @@ // Register service worker for offline content caching if ('serviceWorker' in navigator) { - navigator.serviceWorker.register('/player/sw.js').then(reg => { + // Explicit scope. The default would be the worker's own directory (/player/), which does NOT + // include /player — the URL the dashboard shows and the one panels are actually configured + // with. Registration succeeded there and then controlled nothing: no shell cache, no content + // cache, no offline playback, and no error to notice. The server sends + // Service-Worker-Allowed so this wider scope is permitted. + navigator.serviceWorker.register('/player/sw.js', { scope: '/' }).then(reg => { console.log('Service Worker registered'); // When a new SW activates, reload so the fresh code takes effect immediately reg.addEventListener('updatefound', () => { diff --git a/server/player/sw.js b/server/player/sw.js index 080f5dc..32be80a 100644 --- a/server/player/sw.js +++ b/server/player/sw.js @@ -1,3 +1,5 @@ +// v22: worker scope widened to '/' (it never controlled /player before) + prune-to-playlist, so a +// replaced asset's superseded copy is reclaimed rather than waiting on the quota. // v21: chunked resumable content prefetch + revision-keyed media URLs — index.html gained // mediaUrl()/requestOfflineCache() and this worker gained the message handler, so an old shell // cache would pair a new worker with a player that never posts it a playlist. @@ -6,7 +8,7 @@ // — a player then ran a new index.html against a stale st-bridge.js and threw on every heartbeat. // Bump whenever a shipped /player asset changes shape; content lives in its own cache, so this // costs a small re-download and never re-fetches the playlist. -const CACHE_NAME = 'rd-player-v21'; +const CACHE_NAME = 'rd-player-v22'; // Content lives in its own cache so the shell can be re-versioned (the activate handler deletes // every cache that is not CACHE_NAME) WITHOUT throwing away megabytes of media that are still // perfectly valid. Rolling the shell used to mean a player re-downloaded its entire playlist. @@ -132,6 +134,14 @@ let prefetchChain = Promise.resolve(); self.addEventListener('message', (event) => { const data = event.data; if (!data || data.type !== 'st-cache-playlist' || !Array.isArray(data.urls)) return; + + // The player sends the COMPLETE set of media this display needs, so anything else in the content + // cache is superseded and can go. Revision-keyed sweeping alone is not enough: replacing an asset + // writes a new randomly-named file, so the old copy lives at a different PATH and nothing keyed + // on the asset path can find it. Without this the cache only grows, and on a panel with a 1GB + // widget quota a handful of replaced videos is the entire budget. + if (data.prune) prefetchChain = prefetchChain.then(() => pruneToPlaylist(data.urls)).catch(() => {}); + for (const url of data.urls) { if (typeof url !== 'string' || !POLICY || !POLICY.isCacheableContent(url, 'GET')) continue; if (prefetching.has(url)) continue; // single-flight: a 60s playlist sweep must not restart it @@ -234,6 +244,18 @@ async function ensureCached(url) { await sweepOldRevisions(cache, url); } +async function pruneToPlaylist(urls) { + const cache = await caches.open(CONTENT_CACHE); + const keep = new Set(urls); + for (const key of await cache.keys()) { + if (keep.has(key.url)) continue; + // An in-flight transfer's bookkeeping belongs to a URL that IS in the keep set; deleting it + // because the chunk key itself is not listed would restart that download on every sweep. + if (POLICY.isInternalKey(key.url) && [...keep].some((u) => POLICY.assetKey(u) === POLICY.assetKey(key.url))) continue; + await cache.delete(key); + } +} + async function dropChunks(cache, url) { for (const key of await cache.keys()) { if (POLICY.isInternalKey(key.url) && POLICY.assetKey(key.url) === POLICY.assetKey(url) && diff --git a/server/server.js b/server/server.js index 40f4b26..b3bccae 100644 --- a/server/server.js +++ b/server/server.js @@ -415,6 +415,16 @@ app.use('/player', express.static(path.join(__dirname, 'player'), { etag: true, if (filePath.endsWith('.js') || filePath.endsWith('.css') || filePath.endsWith('.html')) { res.setHeader('Cache-Control', 'no-cache'); } + if (filePath.endsWith('sw.js')) { + // A worker's default scope is its own directory, so sw.js at /player/sw.js can only control + // /player/ AND BELOW — which does not include /player itself. The player is served at all three + // of /player, /player/ and /player/index.html, and /player (no trailing slash) is the one + // everybody actually uses: it is what the dashboard shows and what gets typed into a panel. + // On that URL the worker registered, reported success, and then controlled nothing at all — no + // shell cache, no content cache, no offline playback, silently. Widening the permitted scope is + // what makes the registration below able to claim the page it was loaded from. + res.setHeader('Service-Worker-Allowed', '/'); + } }})); // Serve setup scripts diff --git a/server/test/player-sw-scope.test.js b/server/test/player-sw-scope.test.js new file mode 100644 index 0000000..b4c7c46 --- /dev/null +++ b/server/test/player-sw-scope.test.js @@ -0,0 +1,76 @@ +'use strict'; + +// The web player's ENTIRE offline story depended on a header nobody had noticed was missing. +// +// A service worker's default scope is its own directory, so /player/sw.js could only ever control +// /player/ and below — which does not include /player itself. The player is served at all three of +// /player, /player/ and /player/index.html, and /player is the one that gets used: it is what the +// dashboard displays and what gets typed into a panel. On that URL registration SUCCEEDED, logged +// "Service Worker registered", and then controlled nothing: no shell cache, no content cache, no +// offline playback. Found by driving a real browser at it; no unit test in the suite could have +// seen it, because the bug lived entirely in the relationship between a URL and a header. +// +// Both halves are pinned here. Drop either one and the player silently stops working offline at the +// URL everyone uses — with no error, on a display nobody is looking at. + +const os = require('node:os'); +const path = require('node:path'); +const fs = require('node:fs'); +const crypto = require('node:crypto'); +process.env.DATA_DIR = path.join(os.tmpdir(), 'st-swscope-' + crypto.randomBytes(4).toString('hex')); +process.env.SELF_HOSTED = 'true'; +process.env.NODE_ENV = 'test'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); + +test('the worker is registered with an explicit root scope', () => { + // Without {scope:'/'} the registration silently narrows to /player/ and the page at /player is + // left uncontrolled. + const html = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8'); + assert.match(html, /navigator\.serviceWorker\.register\('\/player\/sw\.js',\s*\{\s*scope:\s*'\/'\s*\}/, + "register() must ask for a scope wider than the worker's own directory"); +}); + +test('the server permits that scope, or the registration is rejected outright', async () => { + // Service-Worker-Allowed is what lets a worker claim a scope above its own path. Without it the + // register() call above does not merely narrow — it FAILS, which is worse: the player then has no + // worker at all, on every URL. + const http = require('node:http'); + const express = require('express'); + const app = express(); + // The same static mount the server uses, exercised through its real setHeaders callback. + const serverSrc = fs.readFileSync(path.join(__dirname, '..', 'server.js'), 'utf8'); + assert.match(serverSrc, /Service-Worker-Allowed/, + 'server.js must set Service-Worker-Allowed on the worker response'); + + app.use('/player', express.static(path.join(__dirname, '..', 'player'), { + setHeaders: (res, filePath) => { if (filePath.endsWith('sw.js')) res.setHeader('Service-Worker-Allowed', '/'); } + })); + const server = http.createServer(app); + await new Promise((r) => server.listen(0, r)); + const port = server.address().port; + + const headers = await new Promise((resolve, reject) => { + http.get(`http://127.0.0.1:${port}/player/sw.js`, (res) => { res.resume(); resolve(res.headers); }) + .on('error', reject); + }); + server.close(); + assert.equal(headers['service-worker-allowed'], '/'); +}); + +test('the worker prunes to the set the player declares', () => { + // Revision-keyed sweeping is not sufficient on its own: replacing an asset writes a NEW + // randomly-named file, so the superseded copy lives at a different path entirely and nothing + // keyed on the asset path can find it. It would sit in the cache until the quota evicted it — on + // a panel with a 1GB widget quota, a few replaced videos is the whole budget. + const sw = fs.readFileSync(path.join(__dirname, '..', 'player', 'sw.js'), 'utf8'); + assert.match(sw, /function pruneToPlaylist/); + assert.match(sw, /if \(data\.prune\)/, 'the prune must be driven by the player declaring a complete set'); + + const html = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8'); + assert.match(html, /prune:\s*true/); + // ...and the declared set must be the RAW assignments. The split `playlist` omits multi-zone + // items, so pruning against it would delete assets a zone is still playing. + assert.match(html, /requestOfflineCache\(Array\.isArray\(data\.assignments\)/); +});