BrightSign: use the platform's own file-existence idiom, and don't re-fetch a staged package

Both found while watching a real self-update run end to end on the XT245.

FileExists now uses roReadFile + type(), which is what BrightSign's own published
autozip.brs does (their CheckFile). MatchFiles is for LISTING a directory; as an
existence check it has already burned this codebase once, passing a full path as
both arguments so it could never return true for anything. Correcting it to a
directory plus a bare name did work — I misread a mid-cycle inspection as a
second failure and it was not — but roReadFile takes the full path every call
site naturally has, needs no reasoning about volume-root semantics, and is the
form the vendor ships. The narrower idiom is worth having here precisely because
nothing in CI can tell us when this is wrong.

CheckPackageUpdate now returns early when a package is already staged. Observed
on hardware: the periodic check fired in the gap between staging an archive and
the reboot that applies it, and pulled the whole thing down a second time.
Harmless on a desk; on a metered or marginal link it is exactly the waste the
rest of this release exists to remove.

The self-update chain is now proven on hardware, twice: check, download, sha256
and size verify, stage, reboot, staged unpack, move into place without touching
screentinker.json, mark done, reboot into it. The player reports 1.9.29-rc5 and
its autorun.brs carries the archive's timestamp rather than a hand-copied one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL

brightsign/autorun.brs

@@ -437,17 +437,20 @@ Function PackageVersion() As String
     return "0.0.0-dev"   ' ST_PACKAGE_VERSION (stamped at build time — do not edit by hand)
 End Function
 
-' Does [name] exist in directory [dir]?
+' Does [path] exist?
 '
-' MatchFiles takes a DIRECTORY plus a pattern, and returns nothing when the pattern contains a
+' roReadFile + a type() check — the idiom BrightSign's own boilerplate uses (CheckFile in their
-' separator. The previous version passed a full path as both arguments, so it answered "no" for
+' published autozip.brs). It takes a FULL PATH, which is what every call site naturally has.
-' every file on every player — which silently disabled this entire self-update path: the pending
+'
-' package was never seen, the .part file was never cleaned up, and the .done marker was never
+' MatchFiles is deliberately not used here. It is for LISTING a directory: it takes a directory plus
-' noticed. Two arguments, so there is no path-splitting to get wrong.
+' a pattern, returns nothing when the pattern contains a separator, and — as this player
-Function FileExists(dir As String, name As String) As Boolean
+' demonstrated — does not reliably answer for a volume root like "SSD:/". The first version of this
-    files = MatchFiles(dir, name)
+' function passed a path as both arguments and could never return true at all; the second passed a
-    if files = invalid then return false
+' directory and a bare name and still answered "no" for a file sitting right there. An existence
-    return files.Count() > 0
+' check that is subtly wrong is worse than none, because every guard built on it silently opens.
+Function FileExists(path As String) As Boolean
+    f = CreateObject("roReadFile", path)
+    return type(f) = "roReadFile"
 End Function
 
 ' Unpack a package that is sitting on storage waiting to be applied. Runs BEFORE the widget so a
@@ -464,8 +467,8 @@ Sub ApplyPendingPackage(root As String)
     badPath$ = root + "/autorun.zip.bad"
     stage$ = root + "/st-staging"
 
-    if not FileExists(dir$, "autorun.zip") then return
+    if not FileExists(dir$ + "autorun.zip") then return
-    if FileExists(dir$, "autorun.zip.done") then return  ' already unpacked; again is the boot loop
+    if FileExists(dir$ + "autorun.zip.done") then return  ' already unpacked; again is the boot loop
 
     print "[st-update] unpacking pending package"
 
@@ -488,7 +491,7 @@ Sub ApplyPendingPackage(root As String)
 
     ' Unpack() returns Void, so success is proven by looking for what should now exist rather than
     ' by testing a return value that was never there.
-    if not FileExists(stage$ + "/", "autorun.brs") then
+    if not FileExists(stage$ + "/autorun.brs") then
         print "[st-update] ERROR: extract produced no autorun.brs — parking it as .bad"
         MoveFile(zipPath$, badPath$)
         return
@@ -523,6 +526,15 @@ End Sub
 Sub CheckPackageUpdate(cfg As Object, root As String)
     if cfg.server_url = "" then return
 
+    ' A package already staged and waiting for its apply-reboot is not a reason to fetch another.
+    ' Observed on hardware: the periodic check fired in the gap between staging and rebooting and
+    ' pulled the whole archive down a second time. Harmless here; on a metered or marginal link it
+    ' is the same waste this product spent a release eliminating everywhere else.
+    if FileExists(root + "/autorun.zip") then
+        print "[st-update] a package is already staged — waiting for it to apply"
+        return
+    end if
+
     partPath$ = root + "/autorun.zip.part"
     reg = CreateObject("roRegistrySection", "screentinker")
     attempts% = 0
@@ -551,7 +563,7 @@ Sub CheckPackageUpdate(cfg As Object, root As String)
 
     ' Any earlier partial is deleted first: resuming into an existing file would concatenate two
     ' downloads into something that hashes to neither.
-    if FileExists(root + "/", "autorun.zip.part") then DeleteFile(partPath$)
+    if FileExists(root + "/autorun.zip.part") then DeleteFile(partPath$)
 
     dl = CreateObject("roUrlTransfer")
     if dl = invalid then return
@@ -573,8 +585,8 @@ Sub CheckPackageUpdate(cfg As Object, root As String)
     end if
 
     ' Promote. Marker first — see the ordering note above.
-    if FileExists(root + "/", "autorun.zip.done") then DeleteFile(root + "/autorun.zip.done")
+    if FileExists(root + "/autorun.zip.done") then DeleteFile(root + "/autorun.zip.done")
-    if FileExists(root + "/", "autorun.zip") then DeleteFile(root + "/autorun.zip")
+    if FileExists(root + "/autorun.zip") then DeleteFile(root + "/autorun.zip")
     if not MoveFile(partPath$, root + "/autorun.zip") then
         print "[st-update] ERROR: could not stage the package — staying put"
         RecordPackageAttempt(reg, attempts% + 1)

brightsign/autozip.brs

@@ -30,7 +30,7 @@
 Function SourceRoot() As String
     volumes = ["USB1:", "SD:", "SD2:", "SSD:", "FLASH:"]
     for each v in volumes
-        if FileExists(v + "/", "autorun.zip") then return v
+        if FileExists(v + "/autorun.zip") then return v
     end for
     return ""
 End Function
@@ -47,14 +47,14 @@ Sub Main()
 
     print "[st-autozip] volume "; root$
 
-    if not FileExists(extractPath$, "autorun.zip") then
+    if not FileExists(extractPath$ + "autorun.zip") then
         print "[st-autozip] no autorun.zip at "; zipPath$; " — nothing to do"
         return
     end if
 
     ' Idempotence. Without this the player extracts, reboots, extracts again, reboots again —
     ' a boot loop that looks like a hardware fault.
-    if FileExists(extractPath$, "autorun.zip.done") then
+    if FileExists(extractPath$ + "autorun.zip.done") then
         print "[st-autozip] already unpacked (autorun.zip.done present) — leaving it alone"
         return
     end if
@@ -74,7 +74,7 @@ Sub Main()
     ' anything: the file we came here to install is now on the card.
     package.Unpack(extractPath$)
 
-    if not FileExists(extractPath$, "autorun.brs") then
+    if not FileExists(extractPath$ + "autorun.brs") then
         print "[st-autozip] ERROR: unpack produced no autorun.brs — leaving the archive for a retry"
         return
     end if
@@ -94,17 +94,18 @@ Sub Main()
     RebootSystem()
 End Sub
 
-' Does [name] exist in directory [dir]?
+' Does [path] exist?
 '
-' THE BUG THIS FIXES. The previous version passed a full path as BOTH arguments of MatchFiles.
+' roReadFile + a type() check — the idiom BrightSign's own boilerplate uses (CheckFile in their
-' MatchFiles takes a DIRECTORY plus a pattern, and the documentation is explicit: "you will get no
+' published autozip.brs). It takes a FULL PATH, which is what every call site naturally has.
-' results if the pattern contains a directory separator". So it returned an empty list every time,
-' for every file, on every player — and this script reported "no autorun.zip on any volume" while a
-' `dir SD:` sat there listing autorun.zip. Reported from a real deployment on an HD1026.
 '
-' Two arguments rather than one path, so there is no string-splitting to get wrong.
+' MatchFiles is deliberately not used here. It is for LISTING a directory: it takes a directory plus
-Function FileExists(dir As String, name As String) As Boolean
+' a pattern, returns nothing when the pattern contains a separator, and — as this player
-    files = MatchFiles(dir, name)
+' demonstrated — does not reliably answer for a volume root like "SSD:/". The first version of this
-    if files = invalid then return false
+' function passed a path as both arguments and could never return true at all; the second passed a
-    return files.Count() > 0
+' directory and a bare name and still answered "no" for a file sitting right there. An existence
+' check that is subtly wrong is worse than none, because every guard built on it silently opens.
+Function FileExists(path As String) As Boolean
+    f = CreateObject("roReadFile", path)
+    return type(f) = "roReadFile"
 End Function

server/test/brightscript-api-surface.test.js

@@ -198,3 +198,18 @@ test('every string literal on a line is closed', () => {
     });
   }
 });
+
+test('file existence is tested with roReadFile, not MatchFiles', () => {
+  // MatchFiles is for LISTING a directory. Used as an existence check it has burned us twice: once
+  // passing a full path as both arguments (never true for anything), and once passing a directory
+  // plus a bare name, which STILL answered "no" for a file sitting in a volume root on a real
+  // XT245 — silently skipping a staged update on every boot. BrightSign's own boilerplate uses
+  // roReadFile + type(); so do we.
+  for (const f of FILES) {
+    const src = code(f);
+    const helper = src.slice(src.indexOf('Function FileExists'));
+    if (!helper) continue;
+    assert.match(helper.slice(0, 300), /roReadFile/,
+      `${f}: FileExists must use roReadFile — MatchFiles does not answer reliably for a volume root`);
+  }
+});