mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
Fix the worker scope that made web offline playback silently inert, and prune superseded assets
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
This commit is contained in:
parent
684e60fc55
commit
75c1940821
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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) &&
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
76
server/test/player-sw-scope.test.js
Normal file
76
server/test/player-sw-scope.test.js
Normal file
|
|
@ -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\)/);
|
||||
});
|
||||
Loading…
Reference in a new issue