From f201c0bc5348829f981de5f53318c84e5e79c110 Mon Sep 17 00:00:00 2001 From: Adrian Hilber Date: Fri, 30 Oct 2026 18:13:31 +0100 Subject: [PATCH] 1.3.1: the library loses no item (atomic index, restore from item.json) index.json was rewritten in place and a failed write was ignored. When it could not be written, the running player still listed every item, but the file kept an older list; the next start loaded only that, and the first change after it made the loss permanent. A write cut short left a broken file and the next start loaded nothing. A bad entry threw out of the start. - index.json and item.json are written to a temporary file and renamed; a failed write is logged. - A bad entry or a broken index.json is logged and skipped, not the rest. - At start every item folder whose item.json is not in index.json is restored from it (logged), and index.json is written again. - A delete removes item.json first, so a half-removed folder stays gone. - deploy/k8s: strategy Recreate (one player per library). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017ybuzgBaGxuB5UiaE8dRa4 --- CHANGELOG.md | 11 +++ README.md | 2 +- SPECIFICATION.md | 12 +++- deploy/k8s/deployment.yaml | 4 ++ src/media.cpp | 142 ++++++++++++++++++++++++++++--------- tests/test_player.cpp | 87 +++++++++++++++++++++++ 6 files changed, 223 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ca80dd..02c2fb5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## 1.3.1 (unreleased) + +### Fixes + +- **The library no longer loses items it has on disk.** Items whose folders and `item.json` were on disk could be missing from the library after a restart, silently. Three causes, all fixed: + - `index.json` was rewritten in place, and a failed write was ignored: when it could not be written (for example a file the player's user may not write), the running player still listed every item, but the file kept an older list, and the next start loaded only that. The first change after it (a conversion, an upload) then wrote the shorter list, so the items stayed lost. A write that stops halfway (the process killed) left a cut-off file, and the next start loaded no item at all. Now `index.json` and `item.json` are written to a temporary file and renamed over the old one; a failed write is logged (`ERROR library: …`). + - An `index.json` entry with a field of the wrong type stopped the start with an exception. It is now logged and skipped; the other entries load. + - **At start, every item folder whose `item.json` is not in `index.json` is restored from it**, logged as `WARN library: restored () from its item.json`, and `index.json` is written again. This also brings back items lost this way before 1.3.1 (their folders are still there). The start logs `INFO library: N items (M restored from item.json)`. +- A deleted item removes its `item.json` first, so a folder that cannot be removed completely does not come back at the next start. +- `deploy/k8s/deployment.yaml` sets `strategy: Recreate`: with the default rolling update the old and the new pod ran on the same library at once, and the last one to write `index.json` dropped the items the other had added. + ## 1.3.0 (2026-10-30) ### Breaking diff --git a/README.md b/README.md index db9f82e..a43f710 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ Precedence is environment, then the JSON file (`--config`, else `PLAYER_CONFIG`, | `PLAYER_FORMAT` | `1080p50` | Platform format (`1080i25` and `1080i29.97` are top field first; `1080i25bff` and `1080i29.97bff` bottom field first) | | `PLAYER_OUTPUTS` | `2` | Output count, 1..16 | | `PLAYER_AUDIO_CHANNELS` | `16` | Default audio channels | -| `PLAYER_LIBRARY_DIR` | `/data/library` | Mezzanine library | +| `PLAYER_LIBRARY_DIR` | `/data/library` | Mezzanine library; one player per directory (Kubernetes: `strategy: Recreate`). Items missing from its `index.json` are restored from their `item.json` at start | | `PLAYER_IMPORT_DIR` | empty | Watched import directory | | `PLAYER_CONVERT_CONCURRENCY` | `1` | ffmpeg jobs at once | | `PLAYER_RAM_CLIP_MAX_S` | `20` | Longest clip kept in RAM | diff --git a/SPECIFICATION.md b/SPECIFICATION.md index cedab30..1c30c5b 100644 --- a/SPECIFICATION.md +++ b/SPECIFICATION.md @@ -283,7 +283,17 @@ is kept (configurable retention) so changing the platform format later is possib stingers, idents and short clips. - Storage layout under `PLAYER_LIBRARY_DIR` (default `/data/library`): one directory per item with `original.*`, `mezz-.mov` or `frame-.v210`, - `thumb.jpg`, `item.json`. + `thumb.jpg`, `item.json`, and `index.json` (all items, in library order). +- `index.json` and each `item.json` are written to a temporary file and renamed over + the old one (1.3.1): a reader or a restart sees the old or the new file, never a + part, and a failed write is logged (`ERROR library: …`) and leaves the old file. + At start an index entry that cannot be read is logged and skipped, not the entries + after it, and every item folder whose `item.json` is not in `index.json` is restored + from it (`WARN library: restored …`); `index.json` is then written again. The start + logs the item count (`INFO library: N items (M restored from item.json)`). A delete + removes `item.json` first, so a folder that is not removed completely does not come + back. One player per library directory: two processes on one directory each write + their own list (the Kubernetes example uses `strategy: Recreate`). --- diff --git a/deploy/k8s/deployment.yaml b/deploy/k8s/deployment.yaml index 8539419..daad844 100644 --- a/deploy/k8s/deployment.yaml +++ b/deploy/k8s/deployment.yaml @@ -6,6 +6,10 @@ metadata: app: mxl-test-player spec: replicas: 1 + # One player per library: a rolling update ran the old and the new pod on the same volume, and + # the index of one overwrote the other's. + strategy: + type: Recreate selector: matchLabels: app: mxl-test-player diff --git a/src/media.cpp b/src/media.cpp index 41f7c72..25b38c8 100644 --- a/src/media.cpp +++ b/src/media.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include namespace mtp { @@ -42,6 +43,52 @@ bool is_animated_codec(const std::string& codec) { return codec == "gif" || codec == "apng" || codec == "webp"; } +// One entry of index.json, or an item.json. Throws on a field of the wrong type. +LibraryItem item_from_json(const json& el) { + LibraryItem it; + it.id = el.value("id", ""); + it.name = el.value("name", it.id); + it.type = parse_item_type(el.value("type", "video")); + if (el.contains("tags")) { + for (const auto& t : el.at("tags")) it.tags.push_back(t.get()); + } + it.original = el.value("original", json::object()); + it.fit = el.value("fit", "fit"); + it.fps_mode = el.value("fps_mode", "drop"); + it.alpha_mode = el.value("alpha_mode", "straight"); + it.loudness = el.value("loudness", false); + it.crossfade_ms = el.value("crossfade_ms", 0); + it.map_channels = el.value("map_channels", 0); + it.conversions = el.value("conversions", json::object()); + return it; +} + +// Written to a temporary file next to `path` and renamed over it: a reader sees the old file or +// the new one, never a part of one, and a file that cannot be written leaves the old one as it +// was. A failure is logged (it was silent before 1.3.1). +bool write_json_atomic(const fs::path& path, const json& j) { + // Unique per write: two processes on one library (an old and a new pod) never share it. + const auto tmp = fs::path(path.string() + ".tmp-" + std::to_string(std::random_device{}())); + std::error_code ec; + { + std::ofstream out(tmp, std::ios::trunc); + out << j.dump(2, ' ', false, json::error_handler_t::replace); // invalid UTF-8 does not throw + out.close(); + if (!out) { + log_error("library: cannot write " + tmp.string() + "; " + path.filename().string() + " is not updated"); + fs::remove(tmp, ec); + return false; + } + } + fs::rename(tmp, path, ec); + if (ec) { + log_error("library: cannot replace " + path.string() + " (" + ec.message() + ")"); + fs::remove(tmp, ec); + return false; + } + return true; +} + std::uint64_t dir_bytes(const fs::path& p) { std::uint64_t n = 0; std::error_code ec; @@ -135,51 +182,78 @@ void Library::stop() { } void Library::load_index() { - const auto path = fs::path(cfg_.library_dir) / "index.json"; - std::ifstream in(path); - if (!in) return; - json j; - try { - in >> j; - } catch (...) { - return; + // A bad entry or a bad index.json is logged and skipped, never the rest with it; an item + // that is not in index.json (an entry skipped here, or one a failed write of index.json left + // out) is restored from its own item.json. + const auto dir = fs::path(cfg_.library_dir); + const auto path = dir / "index.json"; + auto add = [&](LibraryItem it) { + // Before 1.0.3 a re-import appended the same id again; keep the first entry. + const bool known = std::any_of(items_.begin(), items_.end(), [&](const LibraryItem& o) { return o.id == it.id; }); + if (it.id.empty() || known) return false; + items_.push_back(std::move(it)); + return true; + }; + json j = json::array(); + if (std::ifstream in(path); in) { + try { + in >> j; + } catch (const std::exception& ex) { + log_error("library: " + path.string() + " is not valid JSON (" + ex.what() + "); items are restored from their item.json"); + j = json::array(); + } } - if (!j.is_array()) return; + if (!j.is_array()) { + log_error("library: " + path.string() + " is not a JSON array; items are restored from their item.json"); + j = json::array(); + } + std::size_t n = 0; for (const auto& el : j) { - LibraryItem it; - it.id = el.value("id", ""); - it.name = el.value("name", it.id); - it.type = parse_item_type(el.value("type", "video")); - if (el.contains("tags")) { - for (const auto& t : el.at("tags")) it.tags.push_back(t.get()); + try { + add(item_from_json(el)); + } catch (const std::exception& ex) { + log_warn("library: entry " + std::to_string(n) + " of index.json skipped (" + ex.what() + ")"); } - it.original = el.value("original", json::object()); - it.fit = el.value("fit", "fit"); - it.fps_mode = el.value("fps_mode", "drop"); - it.alpha_mode = el.value("alpha_mode", "straight"); - it.loudness = el.value("loudness", false); - it.crossfade_ms = el.value("crossfade_ms", 0); - it.map_channels = el.value("map_channels", 0); - it.conversions = el.value("conversions", json::object()); - // Before 1.0.3 a re-import appended the same id again; keep the first entry. - const bool known = std::any_of(items_.begin(), items_.end(), [&](const LibraryItem& o) { return o.id == it.id; }); - if (!it.id.empty() && !known) items_.push_back(std::move(it)); + ++n; + } + std::vector dirs; + std::error_code ec; + for (const auto& ent : fs::directory_iterator(dir, ec)) { + if (ent.is_directory(ec) && fs::exists(ent.path() / "item.json", ec)) dirs.push_back(ent.path()); + // A temporary index left by a write that did not finish. + else if (ent.path().filename().string().rfind("index.json.tmp", 0) == 0) fs::remove(ent.path(), ec); } + std::sort(dirs.begin(), dirs.end()); + std::size_t restored = 0; + for (const auto& d : dirs) { + try { + std::ifstream in(d / "item.json"); + json el; + in >> el; + auto it = item_from_json(el); + if (it.id != d.filename().string()) continue; // not an item folder of its own + const auto name = it.name; + if (add(std::move(it))) { + ++restored; + log_warn("library: restored " + d.filename().string() + " (" + name + ") from its item.json; it was missing from index.json"); + } + } catch (const std::exception& ex) { + log_warn("library: " + (d / "item.json").string() + " not read (" + ex.what() + ")"); + } + } + if (restored > 0) save_index(); + log_info("library: " + std::to_string(items_.size()) + " items (" + std::to_string(restored) + " restored from item.json)"); bytes_ = dir_bytes(cfg_.library_dir); } +// Callers hold mu_ (or are the constructor): one writer of index.json at a time in a process. void Library::save_index() const { json arr = json::array(); for (const auto& it : items_) arr.push_back(to_json(it)); - const auto path = fs::path(cfg_.library_dir) / "index.json"; - std::ofstream out(path); - out << arr.dump(2); + write_json_atomic(fs::path(cfg_.library_dir) / "index.json", arr); } -void Library::save_item(const LibraryItem& it) const { - std::ofstream out(fs::path(item_dir(it.id)) / "item.json"); - out << to_json(it).dump(2); -} +void Library::save_item(const LibraryItem& it) const { write_json_atomic(fs::path(item_dir(it.id)) / "item.json", to_json(it)); } nlohmann::json Library::to_json(const LibraryItem& it) const { return json{{"id", it.id}, @@ -349,6 +423,8 @@ bool Library::remove(const std::string& id, std::string& error) { return false; } std::error_code ec; + // item.json first: a folder that is not removed completely is not restored at the next start. + fs::remove(fs::path(item_dir(id)) / "item.json", ec); fs::remove_all(item_dir(id), ec); items_.erase(it); save_index(); diff --git a/tests/test_player.cpp b/tests/test_player.cpp index 0685617..3bb824e 100644 --- a/tests/test_player.cpp +++ b/tests/test_player.cpp @@ -387,6 +387,93 @@ TEST_CASE("importing the same file again keeps one library item") { std::filesystem::remove_all(dir); } +TEST_CASE("the library loses no item: a bad index entry, a stale or broken index.json, a read-only index.json") { + namespace fs = std::filesystem; + const auto dir = fs::temp_directory_path() / ("mtp-library-index-" + std::to_string(::getpid())); + fs::remove_all(dir); + fs::create_directories(dir / "import"); + Config cfg; + cfg.library_dir = (dir / "library").string(); + const VideoFormat fmt = *parse_format("1080p50"); + const auto index = dir / "library" / "index.json"; + std::vector ids; + { + Library library(cfg); // not started: no conversion workers + for (const char* n : {"a.bin", "b.bin", "c.bin", "d.bin"}) { + const auto file = (dir / "import" / n).string(); + std::ofstream(file, std::ios::binary) << "not really a clip " << n; + ids.push_back(library.ingest_file(file, n, nlohmann::json::object(), fmt).id); + } + } + auto read_index = [&] { + std::ifstream in(index); + return nlohmann::json::parse(in); + }; + auto ids_of = [](const std::vector& items) { + std::vector out; + for (const auto& it : items) out.push_back(it.id); + std::sort(out.begin(), out.end()); + return out; + }; + auto sorted = ids; + std::sort(sorted.begin(), sorted.end()); + const auto full = read_index(); + REQUIRE(full.size() == 4); + + // An entry with a field of the wrong type is skipped (and logged), not the entries after it + // (before 1.3.1 it threw out of the constructor); its item.json brings the item back. + auto bad = full; + bad[1]["crossfade_ms"] = "zero"; + bad[1]["name"] = "renamed in the index only"; + std::ofstream(index) << bad.dump(); + { + Library library(cfg); + CHECK(ids_of(library.list()) == sorted); + CHECK(library.get(ids[1]).name == "b.bin"); // from item.json + CHECK(library.get(ids[0]).name == "a.bin"); + } + // index.json that lists only some items (a write that did not reach it): the others come back + // from their item.json, and index.json lists all again. + std::ofstream(index) << nlohmann::json::array({full[0]}).dump(); + { + Library library(cfg); + CHECK(ids_of(library.list()) == sorted); + CHECK(library.list().front().id == ids[0]); // index.json order first + } + CHECK(read_index().size() == 4); + // index.json cut short or empty (a write interrupted after it truncated the file). + for (const std::string cut : {full.dump(2).substr(0, full.dump(2).size() / 2), std::string()}) { + std::ofstream(index) << cut; + Library library(cfg); + CHECK(ids_of(library.list()) == sorted); + } + // A deleted item does not come back. + { + Library library(cfg); + std::string err; + REQUIRE(library.remove(ids[3], err)); + } + { + Library library(cfg); + CHECK(library.list().size() == 3); + CHECK_FALSE(library.exists(ids[3])); + } + // index.json is replaced, not written in place: a read-only index.json (a file this user may + // not write) is still updated, and no temporary file is left. + if (::geteuid() != 0) { + fs::permissions(index, fs::perms::owner_read | fs::perms::group_read | fs::perms::others_read); + { + Library library(cfg); + const auto file = (dir / "import" / "e.bin").string(); + std::ofstream(file, std::ios::binary) << "not really a clip e"; + library.ingest_file(file, "e.bin", nlohmann::json::object(), fmt); + } + CHECK(read_index().size() == 4); + } + for (const auto& ent : fs::directory_iterator(dir / "library")) CHECK(ent.path().filename().string().find(".tmp") == std::string::npos); + fs::remove_all(dir); +} + TEST_CASE("an animation plays at its own frame timing, mapped onto whole output frames") { // 10 frames of 0.1 s: 1 s per loop. const std::vector even(10, 0.1);