screentinker/server/test/player-media-error-multiadvance.test.js
ScreenTinker 24e430b354 One broken clip, one skip: stop media errors advancing the playlist N times
Found the day the live debug log started working, which is the only reason anyone
saw it. A BrightSign XT245 playing a 40s clip as a SINGLE-item playlist logged four
`Video error` events at every loop boundary and then three back-to-back "Playing:"
lines, with `play() rejected AbortError` and `muted-fallback play() also failed` in
between as the second mount aborted the first. On a one-item playlist that just
re-plays the same file, so it looked like nothing.

On a real playlist the identical storm skips one item per surplus event. Silently.
The operator sees a playlist that drops content and nothing says why. Same family as
234.

Two independent defects produced it:

1. `video.onerror` had no once-guard — its sibling in the buffered path has
   `if (done) return`, this one didn't — so every event scheduled its own nextItem.

2. Every call site wrote `advanceTimer = setTimeout(...)` DIRECTLY. A second write
   before the first fired ORPHANED the earlier timer instead of cancelling it: still
   pending, no longer referenced, so renderContent's clearTimeout could only ever
   cancel the last one. All the others fired. That made a dozen sites capable of
   leaking a timer, not just the error handlers — so the fix is a scheduleAdvance()
   helper that clears before it arms, and a test asserting nothing assigns the timer
   directly ever again.

The four error handlers (buffered/non-buffered x video/image) had drifted apart
because they were four copies; they now share one mediaFailureSkip(), which also
reports the actual MediaError code. The old line logged the DOM event
({"isTrusted":true}) and never touched el.error, so the log could say a video failed
but never why.

Third guard: an element that is still playable is not a failure. `error` fires with
el.error set; an event carrying no MediaError against an element with frames buffered
ahead of it did not fail at anything, and discarding a healthy item on that basis is
worse than the event being reacted to. Anything genuinely unplayable (no MediaError
AND nothing decoded) is still skipped, so a broken clip can never stall the playlist.

Verified on the XT245: 150s of playback went from 2-3 advances and an AbortError pair
per loop boundary to exactly one advance and zero AbortErrors, and the surviving
diagnostic now names the real cause -- `code=3 DECODE`, four raw error events
collapsing to one reported failure.

All three guards are mutation-tested: removing any one of them fails a test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014skWYXJUWhF73EvNPgB2AS
2026-08-07 19:12:16 -05:00

190 lines
9.2 KiB
JavaScript

'use strict';
/*
* A media error used to advance the playlist once PER ERROR EVENT.
*
* Found on a BrightSign XT245 the day the live debug log started working, which is the only reason
* anyone saw it: a 40s clip on a SINGLE-item playlist logged four `Video error` events at each loop
* boundary and then three back-to-back "Playing:" lines, with `play() rejected AbortError` and
* `muted-fallback play() also failed` in between as the second mount aborted the first. On a
* one-item playlist that just re-plays the same file, so it looked like nothing.
*
* On a REAL playlist the identical storm skips one item per surplus event. Silently. The operator
* sees a playlist that drops content and nothing anywhere says why.
*
* Two independent defects produced it:
*
* 1. `video.onerror` had no once-guard (its sibling in the buffered path has `if (done) return`),
* so every event scheduled its own `nextItem`.
* 2. Every call site wrote `advanceTimer = setTimeout(...)` DIRECTLY. A second write before the
* first fired ORPHANED the earlier timer rather than cancelling it — still pending, no longer
* referenced, so `renderContent`'s `clearTimeout(advanceTimer)` could only ever cancel the
* last one. All the others fired.
*
* (2) is the more dangerous half: it made every one of a dozen call sites capable of leaking a
* timer, not just the error handlers.
*/
const { test } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const path = require('path');
const ROOT = path.join(__dirname, '..', '..');
const PLAYER = fs.readFileSync(path.join(ROOT, 'server/player/index.html'), 'utf8');
/*
* Run the real helpers with a fake clock, so "how many advances happened" is a fact rather than a
* reading of the source.
*/
function harness() {
const src = PLAYER.slice(PLAYER.indexOf('let advanceTimer = null;'), PLAYER.indexOf('// Buffered widget swap'));
const timers = new Map();
let seq = 0;
const advances = [];
const logs = [];
const env = {
setTimeout: (fn, ms) => { const id = ++seq; timers.set(id, { fn, ms }); return id; }, // seq = total ever armed
clearTimeout: (id) => { timers.delete(id); },
nextItem: () => advances.push(true),
console: { error: (m) => logs.push(['e', m]), warn: (m) => logs.push(['w', m]), log: (m) => logs.push(['i', m]) },
};
const api = new Function(
'setTimeout', 'clearTimeout', 'nextItem', 'console',
`${src}; return { scheduleAdvance, mediaFailureSkip, pending: () => arguments };`,
)(env.setTimeout, env.clearTimeout, env.nextItem, env.console);
return {
...api,
advances,
logs,
fireAll() { for (const [id, t] of [...timers]) { timers.delete(id); t.fn(); } },
pendingCount: () => timers.size,
armedEver: () => seq,
};
}
const mediaError = (code) => ({ code, message: '' });
test('four error events on one clip cause ONE advance, not four', () => {
// The exact sequence the XT245 produced.
const h = harness();
const video = { error: mediaError(3), readyState: 0 };
for (let i = 0; i < 4; i++) h.mediaFailureSkip(video, 'video', 'clip.mp4');
assert.equal(h.pendingCount(), 1, 'only one advance may be pending');
h.fireAll();
assert.equal(h.advances.length, 1, `four errors advanced the playlist ${h.advances.length} times`);
});
test('...which is what stops a real playlist silently dropping items', () => {
// Stated separately because this is the customer-visible consequence, and it is the reason the
// one-item repro was worth chasing at all.
const h = harness();
const video = { error: mediaError(2), readyState: 0 };
for (let i = 0; i < 7; i++) h.mediaFailureSkip(video, 'video', 'clip.mp4');
h.fireAll();
assert.equal(h.advances.length, 1, 'a 10-item playlist would otherwise skip 6 items');
});
test('the FIRST failure wins: later events cannot postpone the skip', () => {
/*
* This is what the once-guard buys that clear-before-arm does not.
*
* `scheduleAdvance` cancels the outgoing timer, so N errors already collapse to ONE advance. But
* without the guard, each error also RE-ARMS the 3s skip — so a clip erroring faster than every
* 3 seconds pushes its own skip out forever and wedges the playlist on a broken item, which is
* the exact failure the 3s skip exists to prevent. Counting armings, not advances, is the only
* assertion that can tell those two implementations apart.
*/
const h = harness();
const video = { error: mediaError(3), readyState: 0 };
for (let i = 0; i < 4; i++) h.mediaFailureSkip(video, 'video', 'clip.mp4');
assert.equal(h.armedEver(), 1, `the skip must be armed once, not re-armed per event (armed ${h.armedEver()}x)`);
});
test('and it is reported once, not once per event', () => {
// The live debug log is fed by console.*, so a handler that logs per event turns one broken clip
// into a flood in the panel an operator opened to find it.
const h = harness();
const video = { error: mediaError(3), readyState: 0 };
for (let i = 0; i < 6; i++) h.mediaFailureSkip(video, 'video', 'clip.mp4');
assert.equal(h.logs.filter(([lv]) => lv === 'e').length, 1);
});
test('the MediaError code is reported, because "Video error" alone explains nothing', () => {
// The original line logged the DOM event ({"isTrusted":true}) and never touched el.error, so the
// live log could say a video failed but never why.
const h = harness();
h.mediaFailureSkip({ error: mediaError(3), readyState: 0 }, 'video', 'clip.mp4');
const line = h.logs.find(([lv]) => lv === 'e')[1];
assert.match(line, /code=3/);
assert.match(line, /DECODE/, 'the code must be named — nobody remembers the MediaError numbers');
assert.match(line, /clip\.mp4/);
});
test('a playable element that raises a bare error is NOT thrown away', () => {
// `error` fires with el.error set. An event carrying no MediaError against an element with
// frames buffered ahead of it did not fail at anything, and discarding a healthy item on that
// basis is worse than the event being reacted to.
const h = harness();
h.mediaFailureSkip({ error: null, readyState: 4 }, 'video', 'clip.mp4');
h.fireAll();
assert.equal(h.advances.length, 0, 'a still-playing video must not be skipped');
assert.match(h.logs.find(([lv]) => lv === 'w')[1], /no MediaError while playable/, 'but it must be visible');
});
test('THE BICONDITIONAL: something genuinely unplayable is still skipped', () => {
// The guard above must not become a way for a broken clip to stall the playlist forever. No
// MediaError AND nothing decoded is a failure.
const h = harness();
h.mediaFailureSkip({ error: null, readyState: 0 }, 'video', 'clip.mp4');
h.fireAll();
assert.equal(h.advances.length, 1, 'an undecodable clip must never wedge the playlist');
});
test('the load/watchdog path has no element and still skips', () => {
const h = harness();
h.mediaFailureSkip(null, 'image', 'broken.png');
h.fireAll();
assert.equal(h.advances.length, 1);
});
test('a follower is told nothing to advance — the leader drives it', () => {
const h = harness();
h.mediaFailureSkip({ error: mediaError(4), readyState: 0 }, 'video', 'clip.mp4', false);
h.fireAll();
assert.equal(h.advances.length, 0, 'a follower advancing itself would desync the wall');
assert.ok(h.logs.some(([lv]) => lv === 'e'), 'but the failure is still reported');
});
// ---------------------------------------------------------------- the orphaned-timer class
test('arming twice cancels the first timer instead of orphaning it', () => {
const h = harness();
h.scheduleAdvance(() => {}, 1000);
h.scheduleAdvance(() => {}, 2000);
assert.equal(h.pendingCount(), 1, 'the first timer must be cancelled, not abandoned still-pending');
});
test('every call site goes through the helper — one leak is enough to bring the bug back', () => {
// The generic fix. A dozen sites wrote the timer directly; any one of them re-introduced by hand
// restores a timer that renderContent cannot cancel.
const body = PLAYER.slice(PLAYER.indexOf('function scheduleAdvance'));
const direct = body.split('\n').filter((l) => /advanceTimer\s*=\s*setTimeout\(/.test(l));
assert.equal(direct.length, 1, `only scheduleAdvance itself may assign the timer; found ${direct.length}:\n${direct.join('\n')}`);
assert.match(PLAYER.slice(PLAYER.indexOf('function scheduleAdvance'), PLAYER.indexOf('const MEDIA_ERR_NAME')),
/if \(advanceTimer\) clearTimeout\(advanceTimer\)/, 'the helper must clear before it arms');
});
test('every media error handler goes through the shared skip', () => {
// Four handlers existed (buffered + non-buffered, video + image) and they had drifted apart:
// only one carried a once-guard. Four copies is how they drifted in the first place.
for (const marker of ["video.onerror = () => mediaFailureSkip(video, 'video', src)",
"img.onerror = () => mediaFailureSkip(img, 'image', src, !isFollower)",
"mediaFailureSkip(video, 'video(buffered)', src)",
"mediaFailureSkip(null, 'image', src)"]) {
assert.ok(PLAYER.includes(marker), `handler not routed through the shared skip: ${marker}`);
}
assert.ok(!/console\.error\('Video error:'/.test(PLAYER), 'the old unguarded handler must be gone');
assert.ok(!/console\.error\('Image error'\)/.test(PLAYER), 'and its image twin');
});