From f912a35a32094d190866dbaa49bf94c4e7d935e0 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 20:16:31 -0700 Subject: [PATCH] Detect helper failures in the Resolve scripts Resolve runs Lua 5.1/LuaJIT, where a popen handle's close() returns only true, so helper failures went unnoticed: a crashed analysis surfaced as a missing temporary file and a failed click-track render imported the previous WAV. The shell now echoes a per-run success marker only when the helper exits 0; a missing marker or a non-zero status from Lua 5.2+ is reported with the helper's own error line instead of the traceback. The analysis file is loaded without globals and always removed, and temporary names add a counter and a once-seeded random token so runs in the same second cannot collide. Output names only replace the extension of the file name, not a dot in a folder name. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + .../Modules/OpenBeat/OpenBeatCommon.lua | 165 +++++++++++++++-- tests/lua/test_helper.lua | 167 ++++++++++++++++++ tests/lua/test_workflow.lua | 5 +- 4 files changed, 320 insertions(+), 18 deletions(-) create mode 100644 tests/lua/test_helper.lua diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fb9c5d..8b47f24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable OpenBeat changes should be summarized here for users. GitHub Release - Installer updates now prepare and verify the new files before replacing the installed ones. If an update fails or is interrupted, the previous installation is kept or restored by running the installer again. - Release builds use one resolved commit for testing, building, and tagging, and pinned build dependencies. +- When beat analysis or click-track rendering fails, OpenBeat now shows the helper's error instead of a missing-file error, and no longer imports an older click track as if it were new. ## 0.0.2 - 2026-04-19 diff --git a/resolve/Fusion/Modules/OpenBeat/OpenBeatCommon.lua b/resolve/Fusion/Modules/OpenBeat/OpenBeatCommon.lua index 431ffbb..c3177e5 100644 --- a/resolve/Fusion/Modules/OpenBeat/OpenBeatCommon.lua +++ b/resolve/Fusion/Modules/OpenBeat/OpenBeatCommon.lua @@ -113,29 +113,125 @@ local function log(message) handle:close() end +local random_seeded = false + +local function seed_random() + if random_seeded then + return + end + random_seeded = true + -- A fresh table's address varies between processes (ASLR), unlike os.time() alone. + local address = tonumber(tostring({}):match("(%x+)$") or "0", 16) or 0 + math.randomseed((os.time() + math.floor(os.clock() * 1000000) + address) % 2147483647) + for _ = 1, 4 do + math.random() + end +end + +local function random_token() + seed_random() + return string.format("%06x%06x", math.random(0, 0xffffff), math.random(0, 0xffffff)) +end + +-- Lua 5.1 and LuaJIT, which Resolve runs, return only `true` from a popen handle's +-- close(), so the helper's exit status is not available there. The shell echoes this +-- marker only when the helper exits with status 0 (`&&` is evaluated at run time by +-- both sh and cmd.exe), so a missing marker means the helper failed. +local SUCCESS_MARKER = "OPENBEAT_HELPER_OK" +local MAX_DETAIL_LENGTH = 600 + +-- The helper's error, without the Python traceback frames around it. +local function failure_detail(output) + local lines = {} + local traceback = false + for line in tostring(output or ""):gmatch("[^\r\n]+") do + if line:match("^Traceback %(most recent call last%)") then + traceback = true + elseif line:match("^%s") or (traceback and line:match("^%[PYI%-")) then + -- Traceback frames, and the bootloader's note that the script raised. + elseif line:match("%S") then + table.insert(lines, line) + end + end + if #lines == 0 then + return "it printed no error message" + end + local keep = traceback and 1 or 3 + local detail = table.concat(lines, "\n", math.max(1, #lines - keep + 1)) + if #detail > MAX_DETAIL_LENGTH then + detail = "..." .. detail:sub(-MAX_DETAIL_LENGTH) + end + return detail +end + local function run_command(command) log("RUN " .. command) - local popen_command = command .. " 2>&1" + local marker = SUCCESS_MARKER .. "_" .. random_token() + local popen_command = command .. " 2>&1 && echo " .. marker if is_windows() and command:sub(1, 1) == '"' then popen_command = '"' .. popen_command .. '"' end - local handle = assert(io.popen(popen_command, "r")) - local output = handle:read("*a") + local handle, open_error = io.popen(popen_command, "r") + if not handle then + error("OpenBeat could not start its helper: " .. tostring(open_error), 0) + end + local output = handle:read("*a") or "" + -- Lua 5.2+ (and LuaJIT built with 5.2 compatibility) also report the exit status. local _, _, code = handle:close() log(output) - if code ~= 0 and code ~= nil then - error(output) + local helper_output, succeeded = output:gsub("%s*" .. marker .. "%s*$", "") + local failed_code = type(code) == "number" and code ~= 0 and code or nil + if succeeded == 0 or failed_code then + local status = failed_code and (" (exit code " .. tostring(failed_code) .. ")") or "" + error("OpenBeat's helper failed" .. status .. ": " .. failure_detail(helper_output) + .. "\nThe full output is in OpenBeat.log.", 0) + end + return helper_output +end + +local function temp_dir() + local names = is_windows() and { "TEMP", "TMP" } or { "TMPDIR", "TMP", "TEMP" } + for _, name in ipairs(names) do + local value = os.getenv(name) + if value and value ~= "" then + return (value:gsub("[/\\]+$", "")) + end end - return output + if is_windows() then + local local_appdata = os.getenv("LOCALAPPDATA") + if local_appdata and local_appdata ~= "" then + return join_path(local_appdata, "Temp") + end + return "C:\\Windows\\Temp" + end + return "/tmp" end +local temp_counter = 0 + +-- Lua 5.1 cannot create files exclusively, so the name must not be guessable or reused: +-- it combines the time, a per-session counter and a random token. local function temp_path(prefix, extension) - local name = string.format("%s_%d_%d%s", prefix, os.time(), math.random(1000, 9999), extension or "") - local base = os.getenv("TMPDIR") or os.getenv("TEMP") or os.getenv("TMP") - if not base or base == "" then - base = is_windows() and "C:\\Windows\\Temp" or "/tmp" + local base = temp_dir() + for _ = 1, 20 do + temp_counter = temp_counter + 1 + local name = string.format("%s_%d_%d_%s%s", prefix, os.time(), temp_counter, random_token(), extension or "") + local path = join_path(base, name) + if not file_exists(path) then + return path + end end - return join_path(base, name) + error("OpenBeat could not choose an unused temporary file name in " .. base, 0) +end + +local function file_size(path) + local handle = io.open(path, "rb") + if not handle then + return nil + end + local size = handle:seek("end") + handle:close() + return size end local function python_bin() @@ -202,6 +298,25 @@ local function numeric_clip_property(clip, key, fallback) return fallback end +-- The analysis file holds only a data table, so it runs without access to any globals. +local function load_analysis(path) + local chunk, load_error = loadfile(path, "t", {}) + if not chunk then + return nil, load_error + end + if setfenv then + setfenv(chunk, {}) + end + local ok, analysis = pcall(chunk) + if not ok then + return nil, analysis + end + if type(analysis) ~= "table" or type(analysis.raw_beats) ~= "table" or type(analysis.quantized_beats) ~= "table" then + return nil, "the analysis file does not contain beat data" + end + return analysis +end + local function analyze_source(source_path) local output = temp_path("openbeat_analysis", ".lua") local final_command = command_prefix() @@ -209,9 +324,18 @@ local function analyze_source(source_path) .. shell_quote(source_path) .. " --format lua --output " .. shell_quote(output) - run_command(final_command) - local analysis = dofile(output) + local ran, run_error = pcall(run_command, final_command) + local analysis, load_error + if ran then + analysis, load_error = load_analysis(output) + end os.remove(output) + if not ran then + error(run_error, 0) + end + if not analysis then + error("OpenBeat could not read the beat analysis of " .. source_path .. ": " .. tostring(load_error), 0) + end return analysis end @@ -265,9 +389,14 @@ local function create_clip_markers(mode) log(message) end +-- Replaces the extension of the file name only, never a dot in a folder name. local function output_path_for(source_path, suffix, extension) - local base = source_path:gsub("%.[^%.]+$", "") - return base .. suffix .. extension + local folder, name = source_path:match("^(.*[/\\])([^/\\]*)$") + if not folder then + folder, name = "", source_path + end + local stem = name:match("^(.+)%.[^%.]+$") or name + return folder .. stem .. suffix .. extension end local function render_click_track(source_path, mode) @@ -281,6 +410,9 @@ local function render_click_track(source_path, mode) .. " --output " .. shell_quote(output) run_command(command) + if (file_size(output) or 0) == 0 then + error("OpenBeat's helper finished but did not write the click track " .. output, 0) + end return output end @@ -429,7 +561,6 @@ function OpenBeat.run(action, mode) if mode ~= "raw" and mode ~= "quantized" then error("Unknown OpenBeat mode: " .. tostring(mode)) end - math.randomseed(os.time()) local ok, result = pcall(function() if action == "timeline_markers" then return create_timeline_markers(mode) @@ -448,7 +579,7 @@ function OpenBeat.run(action, mode) if not ok then log("ERROR " .. tostring(result)) - error(result) + error(result, 0) end return result diff --git a/tests/lua/test_helper.lua b/tests/lua/test_helper.lua new file mode 100644 index 0000000..1585d27 --- /dev/null +++ b/tests/lua/test_helper.lua @@ -0,0 +1,167 @@ +-- How OpenBeat runs its helper: failures must be detected without an exit status, +-- temporary files must not collide or linger, and output names must keep their folder. +local directory = assert(arg[1]) +local open_file = io.open +io.open = function(path, mode) + if tostring(path):match("OpenBeat%.log$") then return nil end + return open_file(path, mode) +end + +local OpenBeat = dofile("resolve/Fusion/Modules/OpenBeat/OpenBeatCommon.lua") +local function upvalue(fn, name) + for index = 1, 200 do + local key, value = debug.getupvalue(fn, index) + if key == name then return value end + if not key then break end + end + error("Missing upvalue " .. name) +end +local analyze_source = upvalue(upvalue(OpenBeat.run, "create_timeline_markers"), "analyze_source") +local render_click_track = upvalue(upvalue(OpenBeat.run, "create_click_audio"), "render_click_track") +local run_command = upvalue(analyze_source, "run_command") +local temp_path = upvalue(analyze_source, "temp_path") +local output_path_for = upvalue(render_click_track, "output_path_for") + +local function exists(path) + local handle = open_file(path, "rb") + if handle then handle:close() end + return handle ~= nil +end +local function write(path, text) + local handle = assert(open_file(path, "wb")); handle:write(text); handle:close() +end +local function read(path) + local handle = assert(open_file(path, "rb")); local text = handle:read("*a"); handle:close(); return text +end +local function fails(fn, ...) + local ok, message = pcall(fn, ...) + assert(not ok, "Expected a failure") + return tostring(message) +end + +-- Fake helper. `behavior` decides what it writes and prints and how it exits; like the +-- shell, the success marker is printed only when the helper exits 0. +local behavior +local commands = {} +io.popen = function(command) + table.insert(commands, command) + local marker = assert(command:match("&& echo (OPENBEAT_HELPER_OK_%x+)$"), "Missing success marker") + local output_path = assert(command:match("%-%-output '([^']+)'")) + local result = behavior(output_path, command) + local output = result.output or "" + if (result.exit or 0) == 0 then output = output .. marker .. "\r\n" end + return { + read = function() return output end, + close = function() + if result.lua52 then + if (result.exit or 0) == 0 then return true, "exit", 0 end + return nil, "exit", result.exit + end + return true -- Lua 5.1 and LuaJIT + end, + } +end + +local analysis_text = "return { duration_seconds = 2, raw_beats = { 0.5 }, quantized_beats = { 0.5 } }" +local traceback = table.concat({ + "Traceback (most recent call last):", + ' File "openbeat/cli.py", line 154, in analyze_command', + " analysis = load_or_analyze(args.audio)", + "soundfile.LibsndfileError: Error opening 'song.wav': Format not recognised.", + "[PYI-4242:ERROR] Failed to execute script 'cli' due to unhandled exception!", +}, "\n") + +-- Success: the analysis is read and its temporary file removed. +local analysis_path +behavior = function(path) analysis_path = path; write(path, analysis_text); return {} end +local analysis = analyze_source("/Media/song.wav") +assert(analysis.raw_beats[1] == 0.5 and analysis.duration_seconds == 2) +assert(not exists(analysis_path), "Analysis file must be removed") + +-- The analysis file gets no access to globals. +behavior = function(path) write(path, "return { raw_beats = { os and 1 or 0 }, quantized_beats = {} }"); return {} end +assert(analyze_source("/Media/song.wav").raw_beats[1] == 0) + +-- A crashing helper reports its error, not the traceback or a missing temporary file. +behavior = function(path) analysis_path = path; return { output = traceback, exit = 1 } end +local message = fails(analyze_source, "/Media/song.wav") +assert(message:find("Format not recognised", 1, true), message) +assert(not message:find("Traceback", 1, true) and not message:find("cli.py", 1, true), message) +assert(not message:find("PYI", 1, true) and not message:find("cannot open", 1, true), message) +assert(not exists(analysis_path)) + +-- A helper that failed after writing part of its output is still a failure. +behavior = function(path) analysis_path = path; write(path, analysis_text); return { output = "Killed", exit = 137 } end +message = fails(analyze_source, "/Media/song.wav") +assert(message:find("Killed", 1, true), message) +assert(not exists(analysis_path), "Analysis file must be removed after a failure") + +-- An unreadable analysis file is reported and removed. +behavior = function(path) analysis_path = path; write(path, "return {"); return {} end +message = fails(analyze_source, "/Media/song.wav") +assert(message:find("could not read the beat analysis", 1, true), message) +assert(not exists(analysis_path)) +behavior = function(path) analysis_path = path; write(path, "local boom = nil; return boom.beats"); return {} end +message = fails(analyze_source, "/Media/song.wav") +assert(message:find("boom", 1, true), message) +assert(not exists(analysis_path)) + +-- Missing analysis output despite success. +behavior = function() return { output = "done" } end +assert(fails(analyze_source, "/Media/song.wav"):find("could not read the beat analysis", 1, true)) + +-- A runtime that reports exit codes (Lua 5.2+) is honored as well. +behavior = function(path) write(path, analysis_text); return { output = "usage: openbeat\nopenbeat: error: bad", exit = 2, lua52 = true } end +message = fails(analyze_source, "/Media/song.wav") +assert(message:find("exit code 2", 1, true) and message:find("openbeat: error: bad", 1, true), message) +behavior = function(path) write(path, analysis_text); return { lua52 = true } end +assert(analyze_source("/Media/song.wav").quantized_beats[1] == 0.5) + +-- Without a traceback, the last lines of output are shown; empty output is explained. +behavior = function() return { output = "one\ntwo\nthree\nfour\n", exit = 1 } end +message = fails(run_command, "helper --output '/x'") +assert(message:find("two\nthree\nfour", 1, true) and not message:find("one", 1, true), message) +behavior = function() return { exit = 1 } end +assert(fails(run_command, "helper --output '/x'"):find("printed no error message", 1, true)) +behavior = function() return { output = "progress\n" } end +assert(run_command("helper --output '/x'") == "progress") + +-- A failed click track render must not hand back the previous file. +local source = directory .. "/song.wav" +local stale = directory .. "/song.openbeat-clicks.wav" +write(stale, "previous click track") +behavior = function() return { output = "OSError: [Errno 28] No space left on device", exit = 1 } end +message = fails(render_click_track, source, "quantized") +assert(message:find("No space left on device", 1, true), message) +behavior = function(path) write(path, ""); return {} end +assert(fails(render_click_track, source, "quantized"):find("did not write the click track", 1, true)) +behavior = function(path) write(path, "new click track"); return {} end +assert(render_click_track(source, "quantized") == stale and read(stale) == "new click track") + +-- Temporary names never repeat, even within one second, and skip existing files. +local seen = {} +for _ = 1, 200 do + local path = temp_path("openbeat_analysis", ".lua") + assert(not seen[path], "Temporary path repeated: " .. path) + assert(path:match("openbeat_analysis_[^/\\]+%.lua$"), path) + seen[path] = true +end +local getenv = os.getenv +os.getenv = function(name) if name == "TMPDIR" then return directory .. "/" end return getenv(name) end +local first = temp_path("openbeat_analysis", ".lua") +assert(first:sub(1, #directory + 1) == directory .. "/" and not first:find("//", 1, true), first) +os.getenv = getenv + +-- Output names only replace the extension of the file name. +local cases = { + { "/Media/song.mp3", "/Media/song.openbeat-clicks.wav" }, + { "/Media/my.music/track", "/Media/my.music/track.openbeat-clicks.wav" }, + { "C:\\Media\\my.music\\track", "C:\\Media\\my.music\\track.openbeat-clicks.wav" }, + { "C:\\Media\\my.music\\song.final.wav", "C:\\Media\\my.music\\song.final.openbeat-clicks.wav" }, + { "/Media/.hidden", "/Media/.hidden.openbeat-clicks.wav" }, + { "song.wav", "song.openbeat-clicks.wav" }, +} +for _, case in ipairs(cases) do + local actual = output_path_for(case[1], ".openbeat-clicks", ".wav") + assert(actual == case[2], case[1] .. " -> " .. actual) +end diff --git a/tests/lua/test_workflow.lua b/tests/lua/test_workflow.lua index ad76c85..dd17035 100644 --- a/tests/lua/test_workflow.lua +++ b/tests/lua/test_workflow.lua @@ -85,7 +85,10 @@ io.popen = function(command) file:write("mock click audio") end file:close() - return { read = function() return "" end, close = function() return true, "exit", 0 end } + -- Like the shell, print the success marker the command echoes after the helper exits 0. + local marker = assert(command:match("&& echo (OPENBEAT_HELPER_OK_%x+)$")) + -- Lua 5.1 and LuaJIT, as in Resolve, return only true from close(). + return { read = function() return marker .. "\n" end, close = function() return true end } end local OpenBeat = dofile("resolve/Fusion/Modules/OpenBeat/OpenBeatCommon.lua")