diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt index d1b2e42..d82c7c2 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt +++ b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt @@ -111,7 +111,7 @@ class PlaylistController( val item = currentItem ?: return val delay = FollowerExit.resumeDelayMs( isRunning = isRunning, - isImageOrWidget = item.mimeType.startsWith("image/") || item.isWidget, + isImageOrWidget = endsOnTimer(item), slotMs = slotMs(item), elapsedMs = System.currentTimeMillis() - itemStartedAt ) ?: return @@ -220,8 +220,17 @@ class PlaylistController( // In solo playback, don't interrupt it: keep it up, stash the new list, and rotate out on the // next natural advance (video end / image duration). Excludes wallFollower + group-sync, whose // advance is driven by their tick, not next() — deferring there would strand the swap. - if (isRunning && !wallFollower && hasContentOnScreen && currentlyPlayingId != null && - newItems.none { it.contentId == currentlyPlayingId }) { + // An EMPTY new list is never a deferral candidate. Clearing a screen's playlist is an + // explicit "stop showing that" from an operator, not an item rotating out — deferring it + // meant selecting "no playlist" left the old content up indefinitely, which is the opposite + // of what was asked for and looked like the setting had done nothing. + if (PendingSwap.shouldDefer( + isRunning = isRunning, + wallFollower = wallFollower, + hasContentOnScreen = hasContentOnScreen, + currentlyPlayingId = currentlyPlayingId, + newContentIds = newItems.map { it.contentId }, + )) { var succ: String? = null if (items.isNotEmpty()) { for (k in 1..items.size) { @@ -232,11 +241,13 @@ class PlaylistController( pendingItems = newItems pendingSuccessorId = succ Log.i("PlaylistController", "Current item removed but still live — deferring rotation-out (successor=$succ)") + armPendingSwapDeadline() return } // A non-deferred structural update supersedes any pending swap. pendingItems = null pendingSuccessorId = null + cancelPendingSwapDeadline() items.clear() items.addAll(newItems) @@ -330,6 +341,7 @@ class PlaylistController( isRunning = false cancelAdvance() cancelRetry() + cancelPendingSwapDeadline() // else a stopped controller can still fire next() hasContentOnScreen = false pendingItems = null pendingSuccessorId = null @@ -343,6 +355,7 @@ class PlaylistController( // Swap in the stashed list now and continue at the preserved successor (or first playable). pendingItems?.let { p -> pendingItems = null + cancelPendingSwapDeadline() val succ = pendingSuccessorId; pendingSuccessorId = null items.clear(); items.addAll(p) if (items.isEmpty()) { currentIndex = -1; cancelAdvance(); onPlaylistEmpty(); return } @@ -373,6 +386,42 @@ class PlaylistController( next() } + /** + * Items whose turn ends on a TIMER rather than a completion callback. + * + * video/youtube belongs here and did not: it is played by loading an embed into a WebView, which + * fires no completion event, so nothing ever advanced past it. playYoutube() even takes a + * durationSec and never reads it. A playlist containing a YouTube item simply stopped there. + * + * That also stranded #157's deferred swap, which waits for "the next natural advance": assigning + * a different playlist while a YouTube item was on screen deferred forever, so the change looked + * like it had been ignored. Reported as "I assigned Playlist 2 and it kept showing the video". + * + * Local and remote non-YouTube video stay off this list — ExoPlayer reports STATE_ENDED and + * onVideoComplete drives those, and a timer would cut a clip short. + */ + private var pendingSwapRunnable: Runnable? = null + + /** Apply a deferred swap even if no advance arrives — see PENDING_SWAP_DEADLINE_MS. */ + private fun armPendingSwapDeadline() { + cancelPendingSwapDeadline() + pendingSwapRunnable = Runnable { + if (pendingItems != null) { + Log.w("PlaylistController", "Deferred playlist swap never got an advance — applying it now") + next() + } + } + handler.postDelayed(pendingSwapRunnable!!, PendingSwap.DEADLINE_MS) + } + + private fun cancelPendingSwapDeadline() { + pendingSwapRunnable?.let { handler.removeCallbacks(it) } + pendingSwapRunnable = null + } + + private fun endsOnTimer(item: PlaylistItem): Boolean = + ItemTiming.endsOnTimer(item.mimeType, item.isWidget) + private fun playCurrentItem() { cancelAdvance() cancelRetry() @@ -395,7 +444,7 @@ class PlaylistController( // For images and widgets, auto-advance after duration. For videos, wait // for the completion callback. Wall followers never auto-advance — the // leader's wall:sync index drives every switch. - if (!wallFollower && (item.mimeType.startsWith("image/") || item.isWidget)) { + if (!wallFollower && endsOnTimer(item)) { // slotMs() floors a zero/negative duration to 10s (the max(1, duration||10) // contract shared with the web/Tizen players). A raw durationSec*1000 here let a // solo fullscreen widget with duration_sec=0 schedule a 0ms advance -> self-loop. diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt index 483464c..0936f0b 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt +++ b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt @@ -76,3 +76,62 @@ object PlaybackResume { return savedIndex } } + +/** + * #157's deferral: when a playlist update drops the item that is CURRENTLY on screen, we let that + * item finish its turn instead of yanking it, and apply the new list at the next natural advance. + * + * The rule needs two guards it did not have, both found from a customer report where a playlist + * change appeared to be ignored entirely: + * + * 1. An EMPTY new list is not a rotation. Clearing a screen's playlist is an operator saying "stop + * showing that", so it must take effect now. Deferring it left the old content up forever. + * 2. Deferring assumes an advance is coming. A YouTube item never advanced (see endsOnTimer), so + * the pending swap was stranded permanently — the caller must pair this with a deadline. + * + * Pure so the rule can be checked without a device or a WebView. + */ +object PendingSwap { + /** + * How long a deferred swap may wait for "the next natural advance" before it is applied anyway. + * The deferral assumes an advance is coming; YouTube proved it might not be, and any future item + * type that ends on a callback could do the same. Must comfortably clear an ordinary dwell so it + * never pre-empts a normal rotation, while still being short enough that an operator watching + * the screen sees their change land. + */ + const val DEADLINE_MS = 60_000L + + /** + * Whether a playlist update should wait for the current item to finish. + * False means apply it immediately. + */ + fun shouldDefer( + isRunning: Boolean, + wallFollower: Boolean, + hasContentOnScreen: Boolean, + currentlyPlayingId: String?, + newContentIds: List, + ): Boolean { + if (!isRunning || wallFollower || !hasContentOnScreen) return false + if (currentlyPlayingId == null) return false + if (newContentIds.isEmpty()) return false // guard 1: an explicit stop + return !newContentIds.contains(currentlyPlayingId) + } +} + +/** + * Which items end on a TIMER versus a completion callback. + * + * video/youtube was in neither camp and so ended on nothing at all: it is played by loading an embed + * into a WebView, which reports no completion, and no advance was ever armed for it. The item's + * configured duration was passed to the player and dropped on the floor. A playlist containing a + * YouTube item simply stopped there for good, and any pending playlist change stopped with it. + * + * Local and remote video deliberately stay OFF the timer path — the player reports STATE_ENDED for + * those and a timer would cut a clip short at its configured duration. + */ +object ItemTiming { + fun endsOnTimer(mimeType: String, isWidget: Boolean): Boolean = + mimeType.startsWith("image/") || isWidget || mimeType == "video/youtube" +} + diff --git a/android/app/src/test/java/com/remotedisplay/player/player/PendingSwapTest.kt b/android/app/src/test/java/com/remotedisplay/player/player/PendingSwapTest.kt new file mode 100644 index 0000000..a82b2f5 --- /dev/null +++ b/android/app/src/test/java/com/remotedisplay/player/player/PendingSwapTest.kt @@ -0,0 +1,98 @@ +package com.remotedisplay.player.player + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * A customer assigned a different playlist to a screen and the screen kept showing the old content. + * Then they selected "no playlist" — still the old content. Restarting the app showed the new + * content instantly, which ruled out downloads, the network and the server payload. + * + * Two faults met. #157's deferral holds a playlist change until the current item finishes its turn, + * and the item on screen was a YouTube video, which never finished: nothing armed an advance for it, + * so the pending change waited for an event that could not arrive. And "no playlist" went down the + * same deferral path, so the one action that should always take effect immediately did not. + * + * Invariants pinned here: + * - an empty new list is applied at once, never deferred + * - a real rotation still defers, because #157's reason for existing has not changed + * - an item that ends on a timer is recognised as such, YouTube included + */ +class PendingSwapTest { + + private val LIVE = "content-on-screen" + + private fun defer( + newIds: List, + current: String? = LIVE, + isRunning: Boolean = true, + wallFollower: Boolean = false, + hasContent: Boolean = true, + ) = PendingSwap.shouldDefer(isRunning, wallFollower, hasContent, current, newIds) + + @Test fun THE_BUG_selecting_no_playlist_must_not_be_deferred() { + // The decisive observation from the report: "I selected No playlist ... it still showed the + // same video." An empty list is an operator saying stop, not an item rotating out. + assertFalse(defer(newIds = emptyList())) + } + + @Test fun a_genuine_rotation_still_defers_157_must_not_regress() { + // The current item is gone from the new list but other items remain: let it finish. + assertTrue(defer(newIds = listOf("other-a", "other-b"))) + } + + @Test fun a_playlist_that_still_contains_the_live_item_never_defers() { + assertFalse(defer(newIds = listOf(LIVE, "other-a"))) + } + + @Test fun nothing_on_screen_yet_means_apply_immediately() { + // A first load has nothing to protect, so there is nothing to wait for. + assertFalse(defer(newIds = listOf("other-a"), hasContent = false)) + assertFalse(defer(newIds = listOf("other-a"), current = null)) + } + + @Test fun a_stopped_controller_does_not_defer() { + // Otherwise a swap is parked on an instance that will never advance again. + assertFalse(defer(newIds = listOf("other-a"), isRunning = false)) + } + + @Test fun a_wall_follower_does_not_defer_it_obeys_the_leader() { + assertFalse(defer(newIds = listOf("other-a"), wallFollower = true)) + } + + @Test fun the_deferral_deadline_is_long_enough_for_a_normal_item_and_short_enough_to_notice() { + // The deadline is the backstop for "no advance ever arrives". It must clear a typical dwell + // comfortably (or it would cut ordinary items short) while still resolving fast enough that + // an operator watching the screen sees their change land. + val deadline = PendingSwap.DEADLINE_MS + assertTrue("deadline must exceed a common 30s dwell", deadline > 30_000L) + assertTrue("an operator should not wait minutes", deadline <= 120_000L) + } +} + +/** + * The other half of the same report. A YouTube item ended on nothing: no timer was armed for it and + * a WebView embed reports no completion, so it held the screen forever and stranded whatever + * playlist change was waiting behind it. + */ +class ItemTimingTest { + + @Test fun THE_BUG_a_youtube_item_must_end_on_a_timer() { + // Nothing else can end it — a WebView embed fires no completion event. + assertTrue(ItemTiming.endsOnTimer("video/youtube", isWidget = false)) + } + + @Test fun images_and_widgets_are_timed_as_they_always_were() { + assertTrue(ItemTiming.endsOnTimer("image/jpeg", isWidget = false)) + assertTrue(ItemTiming.endsOnTimer("image/png", isWidget = false)) + assertTrue(ItemTiming.endsOnTimer("text/html", isWidget = true)) + } + + @Test fun real_video_must_NOT_be_timed_or_clips_get_cut_short() { + // These end on STATE_ENDED. Arming a timer would truncate a clip at its configured duration, + // which is the regression to avoid while fixing the YouTube case. + assertFalse(ItemTiming.endsOnTimer("video/mp4", isWidget = false)) + assertFalse(ItemTiming.endsOnTimer("video/webm", isWidget = false)) + } +} diff --git a/frontend/js/api.js b/frontend/js/api.js index 9ec7996..9fb8b82 100644 --- a/frontend/js/api.js +++ b/frontend/js/api.js @@ -193,6 +193,7 @@ export const api = { getItemSchedules: (id, itemId) => request(`/playlists/${id}/items/${itemId}/schedules`), setItemSchedules: (id, itemId, blocks) => request(`/playlists/${id}/items/${itemId}/schedules`, { method: 'PUT', body: JSON.stringify({ blocks }) }), assignPlaylistToDevice: (playlistId, device_id) => request(`/playlists/${playlistId}/assign`, { method: 'POST', body: JSON.stringify({ device_id }) }), + clearDevicePlaylist: (device_id) => request(`/devices/${device_id}/playlist`, { method: 'DELETE' }), publishPlaylist: (id) => request(`/playlists/${id}/publish`, { method: 'POST' }), discardPlaylistDraft: (id) => request(`/playlists/${id}/discard`, { method: 'POST' }), diff --git a/frontend/js/i18n/en.js b/frontend/js/i18n/en.js index 9238bc4..d338fa9 100644 --- a/frontend/js/i18n/en.js +++ b/frontend/js/i18n/en.js @@ -524,6 +524,8 @@ export default { 'device.debug.toggle': 'Debug logging (live)', 'device.debug.hint': 'Streams player/zone logs from this device in real time. Turns off on its own when the device reconnects.', 'device.ota.toggle': 'Self-update (OTA)', + 'device.ota.beta': 'Accept pre-release builds', + 'device.ota.beta_hint': 'Keeps this display on a test build instead of updating it back to the current release. Only affects pre-releases of the version already installed — once a newer release ships, this display updates to it normally.', 'device.ota.hint': 'When off, this device is never offered an update — an MDM or operator owns its updates instead. Turn OFF for MDM-managed panels (e.g. Pivot/MAXHUB) so the app never shows a self-install dialog.', 'device.reboot_schedule.label': 'Nightly reboot', 'device.reboot_schedule.hint': 'Reboot this panel once a day at this device-local time (leave blank for off). A clean nightly reboot clears memory leaks and re-syncs the clock. Silent on device-owner panels; a no-op on panels that can\'t self-reboot.', diff --git a/frontend/js/views/device-detail.js b/frontend/js/views/device-detail.js index 07df81b..fad4b94 100644 --- a/frontend/js/views/device-detail.js +++ b/frontend/js/views/device-detail.js @@ -446,6 +446,10 @@ async function loadDevice(deviceId, activeTab = null) { ${t('device.ota.toggle')}
${t('device.ota.hint')}
+ +
${t('device.ota.beta_hint')}
@@ -977,6 +981,7 @@ function setupActions(device) { orientation: document.getElementById('deviceOrientation').value, default_content_id: document.getElementById('deviceDefaultContent').value || null, ota_enabled: document.getElementById('otaToggle')?.checked ? 1 : 0, + ota_beta: document.getElementById('otaBetaToggle')?.checked ? 1 : 0, reboot_schedule: document.getElementById('rebootSchedule')?.value || null, }); showToast(t('device.toast.settings_saved'), 'success'); @@ -1039,10 +1044,15 @@ function setupActions(device) { playlistPicker.addEventListener('change', async () => { const newPlaylistId = playlistPicker.value; - if (!newPlaylistId) return; // Don't allow deselecting for now try { - await api.assignPlaylistToDevice(newPlaylistId, device.id); - device.playlist_id = newPlaylistId; + // Empty value is the "No playlist" option. It used to be discarded right here, so the + // option was offered, selecting it did nothing, and nothing said so (#234). + if (newPlaylistId) { + await api.assignPlaylistToDevice(newPlaylistId, device.id); + } else { + await api.clearDevicePlaylist(device.id); + } + device.playlist_id = newPlaylistId || null; const assignments = await api.getAssignments(device.id); const pc = document.getElementById('playlistContainer'); pc.innerHTML = renderPlaylist(assignments); diff --git a/server/db/database.js b/server/db/database.js index d49470c..e12bc4a 100644 --- a/server/db/database.js +++ b/server/db/database.js @@ -299,6 +299,12 @@ const migrations = [ // device an update (an MDM/operator owns its updates). Default 1 (self-update on). // UPDATE devices SET ota_enabled = 0 WHERE id = ''; (1 to re-enable) "ALTER TABLE devices ADD COLUMN ota_enabled INTEGER NOT NULL DEFAULT 1", + // Opt a single display into pre-release builds. Without this, handing someone a test build is a + // trap: a prerelease sorts BELOW its own release (1.9.25-fix234d < 1.9.25), so the next OTA check + // correctly "upgrades" the device straight back off the build you asked them to test — silently, + // within minutes. It cost a reporter on #234 an evening of testing code that had already been + // replaced under them. Set this and the display keeps a same-core prerelease. + "ALTER TABLE devices ADD COLUMN ota_beta INTEGER NOT NULL DEFAULT 0", // #161: privilege tier reported by the player (0 unprivileged / 1 device-admin / 2 owner-or- // delegated-install) + whether a foreign device owner (MDM) manages it. Drives dashboard gating // of Tier-2 controls (reboot/kiosk/time) — shown only for owned panels. diff --git a/server/lib/ota-breaker.js b/server/lib/ota-breaker.js index b359be0..0c072fd 100644 --- a/server/lib/ota-breaker.js +++ b/server/lib/ota-breaker.js @@ -68,7 +68,7 @@ function isReleased(p) { return p.pre === null || /^patch\d+$/i.test(p.pre); } // decide(clientVersion, latestVersion, deviceId?, now?) -> // { update_available, reason, retry_after_seconds?, log? } -function decide(clientVersion, latestVersion, deviceId = null, now = Date.now()) { +function decide(clientVersion, latestVersion, deviceId = null, now = Date.now(), betaChannel = false) { // ---- PHANTOM / unrecognized guard (immediate, version-based, no rate state) ---- if (!clientVersion) return { update_available: false, reason: 'no-version' }; const pc = parseVer(clientVersion), pl = parseVer(latestVersion); @@ -76,10 +76,24 @@ function decide(clientVersion, latestVersion, deviceId = null, now = Date.now()) const full = cmpParsed(pc, pl); if (full === 0) return { update_available: false, reason: 'up-to-date' }; if (full > 0) return { update_available: false, reason: 'client-newer' }; // never offer a downgrade - if (!isReleased(pc) && coreCmp(pc, pl) < 0) { // GENUINE superseded old-core prerelease (e.g. 1.9.1-beta4) — a -patchN release is NOT one, so it still gets offered + // betaChannel is exempt: this guard would otherwise strand the very displays we hand test + // builds to. A tester on 1.9.25-fix234d has an older core than a released 1.9.26, so without + // the exemption they are told "superseded" forever and never rejoin the release line — the + // opposite of what opting in should mean. Opting in must be reversible by shipping a release. + if (!betaChannel && !isReleased(pc) && coreCmp(pc, pl) < 0) { // GENUINE superseded old-core prerelease (e.g. 1.9.1-beta4) — a -patchN release is NOT one, so it still gets offered return { update_available: false, reason: 'superseded-prerelease', log: logOnce(clientVersion, `[ota] superseded prerelease '${clientVersion}' (older core than latest=${latestVersion}) — no offer`) }; } + // A display opted into pre-release builds keeps a prerelease of the CURRENT core. Semver puts + // 1.9.25-fix234d below 1.9.25, so without this the only "upgrade" on offer is dropping the very + // build we asked this display to run — which is how a test build silently reverts. Scoped to the + // same core on purpose: an older-core prerelease is genuinely stale and still gets offered, and + // once 1.9.26 ships a 1.9.25-anything device is behind and updates normally. So opting in cannot + // strand a display on an abandoned branch. + if (betaChannel && !isReleased(pc) && coreCmp(pc, pl) === 0) { + return { update_available: false, reason: 'beta-channel' }; + } + // ---- offerable (recent real older version) -> RATE breaker, keyed per device / per version ---- const key = deviceId ? 'd:' + deviceId : 'v:' + clientVersion; let b = state.get(key); diff --git a/server/routes/devices.js b/server/routes/devices.js index 30e56b5..faf455a 100644 --- a/server/routes/devices.js +++ b/server/routes/devices.js @@ -215,11 +215,44 @@ router.get('/:id/preview-payload', (req, res) => { }); // Update device +// Clear a device's playlist — the "No playlist" option in the dashboard picker. +// +// There was no way to do this. PUT /devices/:id ignores playlist_id (it always has), and +// POST /playlists/:id/assign can only ever SET one, so the picker carried a guard that +// silently discarded the selection: `if (!newPlaylistId) return; // Don't allow deselecting`. +// The option was offered, selecting it did nothing, and no error said so — reported on #234 +// as "I selected No playlist and it still showed the same video". It did. +// +// Device-scoped rather than playlist-scoped because there is no playlist to authorize +// against when clearing; ownership is checked the same way every other device mutation +// checks it. Clearing an already-clear device is a no-op success, so the button is safe to +// press twice. +router.delete('/:id/playlist', (req, res) => { + const device = checkDeviceOwnership(req, res); + if (!device) return; + + db.prepare('UPDATE devices SET playlist_id = NULL, updated_at = ? WHERE id = ?') + .run(Math.floor(Date.now() / 1000), req.params.id); + + // Push the now-empty playlist so the screen stops, rather than leaving the old content up + // until something else happens to update it. + try { + const io = req.app.get('io'); + if (io) { + const { buildPlaylistPayload } = require('../ws/deviceSocket'); + const commandQueue = require('../lib/command-queue'); + commandQueue.queueOrEmitPlaylistUpdate(io.of('/device'), req.params.id, buildPlaylistPayload); + } + } catch (e) { /* silent — the DB is the source of truth, the push is best-effort */ } + + res.json({ success: true }); +}); + router.put('/:id', (req, res) => { const device = checkDeviceOwnership(req, res); if (!device) return; - const { name, notes, timezone, orientation, default_content_id, layout_id, ota_enabled, reboot_schedule } = req.body; + const { name, notes, timezone, orientation, default_content_id, layout_id, ota_enabled, ota_beta, reboot_schedule } = req.body; // #150: validate orientation against the known enum (previously accepted any string, which // let a bad value reach the player -> unknown rotation falls back to landscape silently). if (orientation !== undefined && !deviceSettings.ORIENTATIONS.has(orientation)) { @@ -249,6 +282,11 @@ router.put('/:id', (req, res) => { if (ota_enabled !== undefined) { updates.push('ota_enabled = ?'); values.push(ota_enabled ? 1 : 0); } + if (ota_beta !== undefined) { + // Per-display pre-release opt-in (#234 follow-up). Stops a test build being reverted by the + // next OTA check, which is what a prerelease version sorting below its own release causes. + updates.push('ota_beta = ?'); values.push(ota_beta ? 1 : 0); + } // #12 scheduled reboot: device-local "HH:MM" (null/'' clears -> off). Reset the // once-per-day guard on any change so a newly-set time can still fire later today. if (reboot_schedule !== undefined) { diff --git a/server/server.js b/server/server.js index bfa2590..e7c4c7d 100644 --- a/server/server.js +++ b/server/server.js @@ -704,6 +704,7 @@ app.get('/api/update/check', (req, res) => { const currentVersion = req.query.version; const deviceId = req.query.device_id || null; // #144: optional; beta4+ clients send it for per-device keying const latestVersion = VERSION; + let betaChannel = false; // per-display pre-release opt-in, set from the device row below // #155/#161: self-update kill switch, enforced SERVER-SIDE so it covers EVERY client // version (not just ones with the client-side stand-down). If OTA is off globally @@ -715,8 +716,12 @@ app.get('/api/update/check', (req, res) => { let otaDeviceOff = false; if (deviceId) { try { - const row = require('./db/database').db.prepare('SELECT ota_enabled FROM devices WHERE id = ?').get(deviceId); + const row = require('./db/database').db.prepare('SELECT ota_enabled, ota_beta FROM devices WHERE id = ?').get(deviceId); otaDeviceOff = !!row && row.ota_enabled === 0; + // #234 follow-up: per-display pre-release opt-in, read from the same row rather than a + // second query. Without it, handing someone a test build is a trap — a prerelease sorts + // BELOW its own release, so the next check "upgrades" the display straight back off it. + betaChannel = !!row && row.ota_beta === 1; } catch (_) { /* device unknown / pre-migration — treat as enabled */ } } if (otaGloballyOff || otaDeviceOff) { @@ -731,7 +736,7 @@ app.get('/api/update/check', (req, res) => { // #144: circuit-breaker + phantom-version guard. Keys per device_id when present, else // per reported version (NOT IP — SNAT). Rate-trips a looping client in seconds. - const verdict = otaBreaker.decide(currentVersion, latestVersion, deviceId); + const verdict = otaBreaker.decide(currentVersion, latestVersion, deviceId, Date.now(), betaChannel); // #146 Item C: EARLY-RETURN before any filesystem work when we won't serve // (rate-backoff, up-to-date, phantom, client-newer, …). A looping client that gets diff --git a/server/test/clear-device-playlist.test.js b/server/test/clear-device-playlist.test.js new file mode 100644 index 0000000..b03b82b --- /dev/null +++ b/server/test/clear-device-playlist.test.js @@ -0,0 +1,107 @@ +'use strict'; + +// "No playlist" was an option you could select that did nothing. +// +// The dashboard picker offered ``, and its change handler +// opened with `if (!newPlaylistId) return; // Don't allow deselecting for now` — so choosing it +// sent no request, changed nothing, and raised no error. The guard was honest about why: there was +// no way to do it. PUT /devices/:id has never read playlist_id (it returns 200 and ignores it), and +// POST /playlists/:id/assign can only set one. +// +// Reported on #234 as "I also selected No playlist ... it still showed the same video". It did. +// +// The invariant: clearing a display's playlist actually clears it, and only the people allowed to +// change that display can do it. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'st-clearpl-')); +process.env.DATA_DIR = tmp; +process.env.JWT_SECRET = 'test-secret-clear-playlist'; + +const express = require('express'); +const { db } = require('../db/database'); +const { requireAuth, generateToken } = require('../middleware/auth'); + +// devices -> workspaces -> organizations -> users, FK-enforced, so seed the whole chain. +function seed(suffix) { + const u = 'u-' + suffix, o = 'o-' + suffix, ws = 'ws-' + suffix; + const dev = 'd-' + suffix, pl = 'p-' + suffix; + db.prepare("INSERT OR IGNORE INTO users (id, email, password_hash, role) VALUES (?, ?, 'x', 'user')") + .run(u, suffix + '@test.local'); + db.prepare('INSERT OR IGNORE INTO organizations (id, name, owner_user_id) VALUES (?, ?, ?)').run(o, 'org ' + suffix, u); + db.prepare('INSERT OR IGNORE INTO workspaces (id, organization_id, name) VALUES (?, ?, ?)').run(ws, o, 'ws ' + suffix); + // accessContext resolves through the MEMBERSHIP tables, not organizations.owner_user_id — + // seeding only the owner column gets a legitimate owner a 403 and looks like an authz bug. + db.prepare("INSERT OR IGNORE INTO organization_members (organization_id, user_id, role) VALUES (?, ?, 'org_owner')").run(o, u); + db.prepare("INSERT INTO playlists (id, name, workspace_id, user_id) VALUES (?, 'PL', ?, ?)").run(pl, ws, u); + db.prepare(`INSERT INTO devices (id, name, workspace_id, user_id, playlist_id, created_at, updated_at) + VALUES (?, 'Screen', ?, ?, ?, strftime('%s','now'), strftime('%s','now'))`).run(dev, ws, u, pl); + return { u, ws, dev, pl }; +} + +const mine = seed('mine'); +const theirs = seed('theirs'); + +const app = express(); +app.use(express.json()); +app.use('/api/devices', requireAuth, require('../routes/devices')); +const server = app.listen(0); + +const userRow = (id) => db.prepare('SELECT id, email, role FROM users WHERE id = ?').get(id); +const tokenFor = (u, ws) => generateToken(userRow(u), ws); + +async function del(deviceId, token) { + await new Promise(r => (server.listening ? r() : server.once('listening', r))); + const res = await fetch(`http://127.0.0.1:${server.address().port}/api/devices/${deviceId}/playlist`, { + method: 'DELETE', + headers: token ? { Authorization: `Bearer ${token}` } : {}, + }); + return res.status; +} + +const playlistOf = (id) => db.prepare('SELECT playlist_id FROM devices WHERE id = ?').get(id).playlist_id; + +test('THE BUG: PUT /devices/:id silently ignores playlist_id, so it could not clear one', async () => { + // Pinned so nobody "fixes" the picker by pointing it back at PUT and re-creating the silence. + const src = fs.readFileSync(path.join(__dirname, '..', 'routes', 'devices.js'), 'utf8'); + const put = src.slice(src.indexOf("router.put('/:id'")); + const body = put.slice(0, put.indexOf('\nrouter.')); + assert.ok(!/playlist_id\s*[,=]/.test(body), 'PUT now touches playlist_id — update this test and the picker'); +}); + +test('THE FIX: clearing a playlist actually clears it', async () => { + assert.equal(playlistOf(mine.dev), mine.pl, 'precondition: a playlist is assigned'); + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null, 'the display must end up with no playlist'); +}); + +test('clearing an already-clear display is a harmless no-op', async () => { + // The button is in a dropdown a person can pick twice; it must not 404 or 500 on the second go. + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null); +}); + +test('someone else\'s display cannot be cleared', async () => { + const before = playlistOf(theirs.dev); + const status = await del(theirs.dev, tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); + assert.equal(playlistOf(theirs.dev), before, 'a refused call must not have changed anything'); +}); + +test('an unauthenticated caller cannot clear a playlist', async () => { + const before = playlistOf(theirs.dev); + assert.equal(await del(theirs.dev), 401); + assert.equal(playlistOf(theirs.dev), before); +}); + +test('a display that does not exist is refused, not invented', async () => { + const status = await del('no-such-device', tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); +}); + +test.after(() => { server.close(); try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} }); diff --git a/server/test/ota-beta-channel.test.js b/server/test/ota-beta-channel.test.js new file mode 100644 index 0000000..572d8c0 --- /dev/null +++ b/server/test/ota-beta-channel.test.js @@ -0,0 +1,79 @@ +'use strict'; + +// Handing someone a test build was a trap. +// +// A prerelease sorts BELOW its own release: 1.9.25-fix234d < 1.9.25. So a display sideloaded with a +// test build asked the server "anything newer?", was correctly told yes — the released 1.9.25 — and +// updated itself straight back off the build we had asked someone to test. Same versionCode, so +// Android installed it without complaint. Silent, within minutes. +// +// It happened on #234: the reporter installed the fix, tested for an evening, and reported that +// nothing had changed. They were right — their tablet was running the old code again by then. +// +// The opt-in is per display and deliberately narrow: it holds a prerelease of the CURRENT core +// only. An older-core prerelease is genuinely stale and must still be offered an update, and once a +// newer release ships the display must rejoin it — otherwise "beta" quietly becomes "abandoned on a +// branch nobody maintains". + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const breaker = require('../lib/ota-breaker'); + +// decide(client, latest, deviceId, now, betaChannel) +const ask = (client, latest, beta) => breaker.decide(client, latest, null, Date.now(), beta); + +test('THE BUG: without the opt-in, a test build is offered its own release and reverts', () => { + const v = ask('1.9.25-fix234d', '1.9.25', false); + assert.equal(v.update_available, true, 'this is the revert that cost a reporter an evening'); + assert.equal(v.reason, 'offer'); +}); + +test('THE FIX: an opted-in display keeps a prerelease of the current release', () => { + const v = ask('1.9.25-fix234d', '1.9.25', true); + assert.equal(v.update_available, false); + assert.equal(v.reason, 'beta-channel'); +}); + +test('opting in does NOT strand a display once a newer release ships', () => { + // The whole risk of a beta flag is that it becomes permanent. 1.9.26 is a real newer core, so an + // opted-in display on any 1.9.25 build must take it. + const v = ask('1.9.25-fix234d', '1.9.26', true); + assert.equal(v.update_available, true, 'a beta display must rejoin the next real release'); +}); + +test('the superseded-prerelease guard is untouched for displays that did NOT opt in', () => { + // #144's phantom protection: a device reporting an ancient beta is not chased with offers. + const v = ask('1.9.1-beta4', '1.9.25', false); + assert.equal(v.update_available, false); + assert.equal(v.reason, 'superseded-prerelease'); +}); + +test('but an opted-in display on an old prerelease IS offered the current release', () => { + // This is the escape hatch, and it is the difference between "beta" and "abandoned". Without + // it the superseded guard pins a tester on an old test build permanently — they would have to + // notice and sideload their way out, which is exactly the trap the opt-in exists to remove. + const v = ask('1.9.1-beta4', '1.9.25', true); + assert.equal(v.update_available, true, 'opting in must never mean never updating again'); +}); + +test('the opt-in changes nothing for a display on a plain release', () => { + assert.equal(ask('1.9.25', '1.9.25', true).reason, 'up-to-date'); + assert.equal(ask('1.9.24', '1.9.25', true).update_available, true, 'a real upgrade is unaffected'); + assert.equal(ask('1.9.24', '1.9.25', false).update_available, true); +}); + +test('a display ahead of the server is never downgraded, opted in or not', () => { + assert.equal(ask('1.9.26', '1.9.25', true).reason, 'client-newer'); + assert.equal(ask('1.9.26', '1.9.25', false).reason, 'client-newer'); +}); + +test('a -patchN build is a release, not a prerelease, so beta does not pin it', () => { + // isReleased() treats patchN as released; it must keep being offered real updates. + const v = ask('1.9.2-patch3', '1.9.25', true); + assert.equal(v.update_available, true, 'a patch release must not be mistaken for a beta build'); +}); + +test('the flag defaults to off, so nothing changes for a fleet that never sets it', () => { + const withDefault = breaker.decide('1.9.25-fix234d', '1.9.25', null, Date.now()); + assert.equal(withDefault.update_available, true, 'default must match pre-existing behaviour'); +});