From 26c059c1b828a14672ae4bb43c8cd76817163ed0 Mon Sep 17 00:00:00 2001 From: screentinker Date: Thu, 13 Aug 2026 20:20:17 -0500 Subject: [PATCH] Every build from alpha10 onward sorted below alpha8 (#270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plain string compare on the prerelease tag put "alpha11" below "alpha8", because '1' < '8'. The OTA check therefore answered client-newer and refused to offer the update — while reporting the newer build as `latest` in the same response: latest_version: 1.9.34-alpha11 current_version: 1.9.34-alpha8 update_available: false reason: "client-newer" So a fleet on alpha8 or alpha9 could not be moved forward at all, silently, and nothing about the symptom pointed at version ordering. alpha10 was never really on offer either; the last update that genuinely worked was alpha6 -> alpha8, where the lexical order happens to be right by luck. This is what semver specifies for a single alphanumeric identifier, and it is simply not what the naming means. lib/version-precedence.js compares digit runs NUMERICALLY, so alpha8 < alpha9 < alpha10 < alpha11, while leaving everything else alphabetical — beta still outranks alpha, rc still outranks beta, and a release still outranks any prerelease of the same core. Dot-separated identifiers are compared per semver and a shorter run loses, so moving the naming to the semver-correct `-alpha.11` form later needs no further change here. TWO comparators carried the assumption, each with a comment asserting lexical was fine "for our naming" — true only while the counter stayed below 10. Both now use the shared helper rather than a third copy drifting into the same trap: - lib/ota-breaker.js the Android OTA path - lib/brightsign-update.js the BrightSign host package, where a wrong-way comparison replaces the script that boots the player lib/ghcr-check.js was checked and is unaffected: it rejects prerelease strings outright rather than ordering them. Tests pin the exact stranding case end to end — decide('1.9.34-alpha8', '1.9.34-alpha11') must be an offer, not client-newer — plus the reverse direction, so a future change cannot merely invert it. 1668/1668 pass. --- server/lib/brightsign-update.js | 6 ++- server/lib/ota-breaker.js | 8 ++- server/lib/version-precedence.js | 62 +++++++++++++++++++++++ server/test/version-precedence.test.js | 68 ++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 server/lib/version-precedence.js create mode 100644 server/test/version-precedence.test.js diff --git a/server/lib/brightsign-update.js b/server/lib/brightsign-update.js index 466549b..23f7480 100644 --- a/server/lib/brightsign-update.js +++ b/server/lib/brightsign-update.js @@ -1,5 +1,7 @@ 'use strict'; +const { preCmp } = require('./version-precedence'); + /* * Should a BrightSign player replace its own host package (autorun.zip)? * @@ -54,7 +56,9 @@ function compareVersions(a, b) { if (A.pre === B.pre) return 0; if (A.pre === null) return 1; // 1.9.29 beats 1.9.29-rc1 if (B.pre === null) return -1; - return A.pre < B.pre ? -1 : 1; // rc1 < rc2, lexicographic is right for our naming + // Natural compare, NOT lexicographic: rc10 must outrank rc9. This file carried the same + // "lexicographic is right for our naming" assumption that broke the Android OTA path. + return preCmp(A.pre, B.pre); } /* diff --git a/server/lib/ota-breaker.js b/server/lib/ota-breaker.js index b55e1d3..325c7f2 100644 --- a/server/lib/ota-breaker.js +++ b/server/lib/ota-breaker.js @@ -41,6 +41,8 @@ const { rollingCounter, bump, read } = require('./rolling-counter'); const rateBackoffCtr = rollingCounter(); // --- minimal semver-ish parse/compare (no dependency) --- +const { preCmp } = require('./version-precedence'); + function parseVer(v) { if (typeof v !== 'string') return null; const m = /^(\d+)\.(\d+)\.(\d+)(?:-(.+))?$/.exec(v.trim()); @@ -54,8 +56,10 @@ function cmpParsed(a, b) { if (a.pre === b.pre) return 0; if (a.pre === null) return 1; // release outranks a prerelease of the same core if (b.pre === null) return -1; - // lexical prerelease compare — fine for beta1..beta9 (cores decide everything else). - return a.pre < b.pre ? -1 : (a.pre > b.pre ? 1 : 0); + // Natural prerelease compare: digit runs numerically, so alpha8 < alpha9 < alpha10 < alpha11. + // A plain lexical compare (what this used to do) put every build from alpha10 onward BELOW + // alpha8, so the check answered client-newer and the fleet could not be moved forward at all. + return preCmp(a.pre, b.pre); } function cmp(a, b) { const pa = parseVer(a), pb = parseVer(b); return (!pa || !pb) ? null : cmpParsed(pa, pb); } diff --git a/server/lib/version-precedence.js b/server/lib/version-precedence.js new file mode 100644 index 0000000..eea4d44 --- /dev/null +++ b/server/lib/version-precedence.js @@ -0,0 +1,62 @@ +'use strict'; + +/* + * Precedence for PRERELEASE identifiers — the `-alpha11` half of `1.9.34-alpha11`. + * + * ⚠️ WHY THIS EXISTS: a plain string compare is what semver specifies for a single alphanumeric + * identifier, and it is wrong for how this project actually names builds. `"alpha11" < "alpha8"` + * because `'1' < '8'`, so EVERY build from alpha10 onward sorted below alpha8 and alpha9. The OTA + * check then answered `client-newer` and refused to offer the update at all — a fleet on alpha8 + * could not be moved forward, silently, with the server reporting the newer build as `latest` in + * the same breath. Two comparators carried the same assumption, both with a comment saying lexical + * was "fine for our naming"; it was fine only while the counter stayed below 10. + * + * The rule here is natural ordering: split each identifier into digit and non-digit runs and + * compare digit runs NUMERICALLY. That gives what a human means by the name — alpha8 < alpha9 < + * alpha10 < alpha11 — while leaving everything else alphabetical, so beta still outranks alpha and + * rc still outranks beta. + * + * Dot-separated identifiers are compared one at a time per semver, and a shorter run of identifiers + * loses when all preceding ones are equal (`alpha` < `alpha.1`), so a future move to the semver- + * correct `-alpha.11` form keeps working without another change here. + * + * Deliberately NOT handled: whether a prerelease outranks a release. That is the caller's rule — + * both callers already implement it, and each has its own exceptions (ota-breaker treats the legacy + * `-patchN` scheme as released). + */ + +// Compare one identifier, digit runs numerically. "alpha10" -> ["alpha", "10"]. +function naturalCmp(x, y) { + const rx = String(x).match(/\d+|\D+/g) || []; + const ry = String(y).match(/\d+|\D+/g) || []; + for (let i = 0; i < Math.max(rx.length, ry.length); i++) { + const a = rx[i], b = ry[i]; + if (a === undefined) return -1; // "alpha" < "alpha1" + if (b === undefined) return 1; + const aNum = /^\d+$/.test(a), bNum = /^\d+$/.test(b); + if (aNum && bNum) { + // Numeric, so 10 beats 8 — the whole point of this file. + if (Number(a) !== Number(b)) return Number(a) < Number(b) ? -1 : 1; + } else if (a !== b) { + // A digit run sorts below a word run, matching semver's numeric-identifiers-first rule. + if (aNum !== bNum) return aNum ? -1 : 1; + return a < b ? -1 : 1; + } + } + return 0; +} + +/* Full prerelease precedence: dot-separated identifiers, each compared naturally. */ +function preCmp(a, b) { + if (a === b) return 0; + const as = String(a).split('.'), bs = String(b).split('.'); + for (let i = 0; i < Math.max(as.length, bs.length); i++) { + if (as[i] === undefined) return -1; // "alpha" < "alpha.1" + if (bs[i] === undefined) return 1; + const c = naturalCmp(as[i], bs[i]); + if (c !== 0) return c; + } + return 0; +} + +module.exports = { preCmp, naturalCmp }; diff --git a/server/test/version-precedence.test.js b/server/test/version-precedence.test.js new file mode 100644 index 0000000..14583d1 --- /dev/null +++ b/server/test/version-precedence.test.js @@ -0,0 +1,68 @@ +'use strict'; + +/* + * Prerelease ordering. + * + * The bug this pins: a plain string compare put every build from alpha10 onward BELOW alpha8, + * because '1' < '8'. The OTA check then answered `client-newer` and refused to offer the update, + * so a fleet on alpha8 could not be moved forward — silently, while the server reported the newer + * build as `latest` in the same response. Two comparators carried the assumption, each with a + * comment saying lexical was fine "for our naming". It was, until the counter passed 9. + */ + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { preCmp } = require('../lib/version-precedence'); +const { cmp } = require('../lib/ota-breaker'); +const bsUpdate = require('../lib/brightsign-update'); + +const sign = (n) => (n === 0 ? 0 : n < 0 ? -1 : 1); + +test('double-digit prereleases outrank single-digit ones', () => { + // The exact case that stranded the fleet. + assert.equal(sign(preCmp('alpha11', 'alpha8')), 1, 'alpha11 must be newer than alpha8'); + assert.equal(sign(preCmp('alpha10', 'alpha9')), 1); + assert.equal(sign(preCmp('beta12', 'beta2')), 1); + assert.equal(sign(preCmp('rc10', 'rc9')), 1); + // And the reverse still holds, so nothing was merely inverted. + assert.equal(sign(preCmp('alpha2', 'alpha10')), -1); +}); + +test('ordinary alphabetical precedence is unchanged', () => { + assert.equal(sign(preCmp('beta1', 'alpha11')), 1, 'beta outranks alpha regardless of number'); + assert.equal(sign(preCmp('rc1', 'beta9')), 1, 'rc outranks beta'); + assert.equal(sign(preCmp('alpha8', 'alpha8')), 0); +}); + +test('semver dot form works too, so the naming can move without another fix', () => { + assert.equal(sign(preCmp('alpha.11', 'alpha.8')), 1); + assert.equal(sign(preCmp('alpha', 'alpha.1')), -1, 'fewer identifiers = lower precedence'); + assert.equal(sign(preCmp('alpha.1', 'beta.1')), -1); +}); + +test('OTA: a device on alpha8 is offered alpha11', () => { + // Through the real comparator the update check uses, not just the helper. + assert.equal(cmp('1.9.34-alpha11', '1.9.34-alpha8'), 1); + assert.equal(cmp('1.9.34-alpha10', '1.9.34-alpha6'), 1); + // A release still outranks any prerelease of the same core. + assert.equal(cmp('1.9.34', '1.9.34-alpha11'), 1); + // And a newer core still wins outright, whatever the prerelease says. + assert.equal(cmp('1.9.35-alpha1', '1.9.34-alpha11'), 1); +}); + +test('OTA decide(): alpha8 -> alpha11 is an offer, not client-newer', () => { + // The end-to-end symptom: the endpoint reported the newer build as `latest` and refused it + // in the same breath. + const { decide } = require('../lib/ota-breaker'); + const d = decide('1.9.34-alpha8', '1.9.34-alpha11', 'test-device-precedence'); + assert.equal(d.update_available, true, `expected an offer, got ${d.reason}`); + assert.notEqual(d.reason, 'client-newer'); +}); + +test('BrightSign host packages order the same way', () => { + // Same assumption lived here, with the same comment. A BrightSign package update that goes + // wrong replaces the script that boots the player, so wrong-way ordering matters more here. + assert.equal(sign(bsUpdate.compareVersions('1.9.34-rc10', '1.9.34-rc9')), 1); + assert.equal(sign(bsUpdate.compareVersions('1.9.34', '1.9.34-rc10')), 1); + assert.equal(sign(bsUpdate.compareVersions('1.9.34-alpha2', '1.9.34-alpha10')), -1); +});