diff --git a/src/actions.cpp b/src/actions.cpp index 84e8ef3..185afea 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -826,10 +826,18 @@ void doBankPruneFolder() { // than proceed with degraded protection. Distinct from "no orphans": the user must // know the prune refused to run and why. if (report.abortedUnreadableUsage) { - ShowConsoleMsg( - "ReaSampler prune: ABORTED -- an instance usage record could not be read.\n" - "Nothing was deleted. Re-opening the project usually clears this (instances " - "republish their usage records on load).\n"); + std::string msg = + "ReaSampler prune: ABORTED -- one or more instance usage records could not " + "be read or decoded. Nothing was deleted.\n" + "If the owning instance is still loaded it will republish its record on the " + "next poll tick, clearing the abort. If the instance no longer exists (the " + "key is an orphaned corrupt record), clear it manually via ReaScript:\n" + " reaper.SetProjExtState(0, \"reasampler\", \"\", \"\")\n" + "Offending key(s):\n"; + for (const std::string& key : report.offendingUsageKeys) { + msg += " " + key + "\n"; + } + ShowConsoleMsg(msg.c_str()); return; } diff --git a/src/persist.cpp b/src/persist.cpp index 67af037..d553925 100644 --- a/src/persist.cpp +++ b/src/persist.cpp @@ -317,6 +317,7 @@ struct PruneScan { std::vector orphans; std::unordered_map sizeByRel; bool abortedUnreadableUsage = false; + std::vector offendingUsageKeys; // non-empty iff abortedUnreadableUsage }; // Non-throwing: readActiveProject + resolveBankFile are pure/string; every filesystem @@ -381,8 +382,10 @@ PruneScan scanPruneOrphans(const BankBook& book, const OwnedFileManifest& owned) // FAIL-SAFE ABORT: a present rsusage_* record could not be read/decoded, so the // protected set is unknowable. Compute NO orphans — every downstream consumer // (dry-run report, confirm set, fresh-recompute delete plan) then deletes - // nothing. The flag surfaces the reason to the action's console message. + // nothing. The flag + key names surface the reason so the action can name each + // offending key for operator recovery. scan.abortedUnreadableUsage = true; + scan.offendingUsageKeys = usage.offendingKeys; return scan; } scan.orphans = pruneOrphans( @@ -402,7 +405,9 @@ PruneReport ReaSamplerSession::pruneDryRun() const { // pS-usage fail-safe: surface the unreadable-record abort so the action halts with // an explicit message instead of reporting "no orphaned files" (the count IS zero — // the scan computed nothing — but the user must know the prune refused to run). + // The offending key names propagate so the action can name each one for recovery. report.abortedUnreadableUsage = scan.abortedUnreadableUsage; + report.offendingUsageKeys = scan.offendingUsageKeys; return report; } diff --git a/src/prune_reconcile.h b/src/prune_reconcile.h index e90ea7a..a44660c 100644 --- a/src/prune_reconcile.h +++ b/src/prune_reconcile.h @@ -70,12 +70,20 @@ namespace reasampler { // must HALT — deleting with degraded protection is the data-loss // direction. Set by the session's scan shell, never by // buildPruneReport (which stays a pure tally). +// * offendingUsageKeys — the exact "rsusage_" ext-state key names that +// triggered the abort (non-empty iff abortedUnreadableUsage). Named +// so the action can print them for operator recovery: a corrupt/ +// oversized key whose owning instance no longer exists is never +// automatically rewritten, so the abort would be permanent without +// a way to clear it. The operator can clear each key via ReaScript: +// reaper.SetProjExtState(0, "reasampler", "", "") struct PruneReport { std::size_t count = 0; std::uint64_t totalBytes = 0; std::vector orphans; bool truncated = false; bool abortedUnreadableUsage = false; + std::vector offendingUsageKeys; // non-empty iff abortedUnreadableUsage }; // The outcome of an actual prune DELETION (Phase R, Wave 3 — R3). The prune shell fills diff --git a/src/sample_usage.cpp b/src/sample_usage.cpp index 62dc3ac..8414564 100644 --- a/src/sample_usage.cpp +++ b/src/sample_usage.cpp @@ -145,11 +145,20 @@ UsagePublishPlan planUsagePublish(const std::optional& existing, } const std::optional theirs = decodeUsageRecord(*existing); if (!theirs) { - // Undecodable existing value under MY OWN key: a sibling sharing this key - // (copy) always writes decodable records, so this is corruption. Overwrite - // with mine — the self-heal restores correct protection for my holds; the - // prune side independently ABORTS while an unreadable record is present - // (foldUsageRecords), so the corrupt window can never cause a delete. + // Undecodable existing value under MY key: corruption (a sibling sharing + // this key via copy always writes decodable records). REMINT rather than + // overwrite: writing mine over the corrupt key would clear the prune-side + // abort, but a same-key sibling B's holds would then be unprotected until + // B publishes again. Leaving the corrupt key in place keeps the prune-side + // abort firing (foldUsageRecords.abortPrune) so the window where B's holds + // might be unprotected can never resolve toward delete. Mine is published + // under the new key that remint produces. + // NOTE (>16 MB gap): readReasamplerExtState returning nullopt for a value + // larger than 16 MB is indistinguishable from "absent" at the publish site; + // that narrow case takes the fresh-write branch above rather than remint. + // Both outcomes are safe (fresh write is also correct for a truly absent key); + // the gap is documented in the header's fail-safe list. + plan.remint = true; return plan; } @@ -235,9 +244,24 @@ UsageFoldResult foldUsageRecords( for (const std::optional& rec : decoded) { if (!rec) { // A present-but-unreadable record: it may protect ANYTHING, so the prune - // must halt outright — heldPaths is irrelevant once abortPrune is set (the - // caller deletes nothing). + // must halt outright. Belt-and-braces: return the PROTECT-ALL set (all + // readable records' paths) so the fail-safe holds even under a future + // caller that forgets to check abortPrune before using heldPaths. The + // abort flag is still the authoritative signal; heldPaths is the + // maximum-protection fallback. result.abortPrune = true; + // Collect EVERY path from EVERY readable record, bypassing the liveness + // filter entirely (on abort the protected set is unknowable, so every + // decoded hold must be included regardless of track-guid membership). + std::unordered_set seen; + for (const std::optional& r : decoded) { + if (!r) continue; + for (const UsageHold& h : r->holds) { + if (h.relativePath.empty()) continue; + if (seen.insert(h.relativePath).second) + result.heldPaths.push_back(h.relativePath); + } + } return result; } records.push_back(*rec); diff --git a/src/sample_usage.h b/src/sample_usage.h index 7ec8af6..3e90fee 100644 --- a/src/sample_usage.h +++ b/src/sample_usage.h @@ -36,7 +36,10 @@ // degrade toward delete); // * unreadable record -> ABORT the prune entirely (foldUsageRecords.abortPrune — a // record we cannot read may protect anything; halting deletes -// nothing). +// nothing). Residual: readReasamplerExtState returning nullopt +// for a >16 MB value is indistinguishable from "absent" at the +// publish site — that narrow case takes the fresh-write branch +// (not remint), noted here for completeness. // // -- Liveness (no stale-key false-protect, no false-delete) -------------------- // @@ -170,10 +173,15 @@ struct UsagePublishPlan { // THIS lifetime's nonce; `mine.unioned` is ignored (the plan computes the written // flag). Branches, in order: // * existing absent/empty -> write mine (unioned=false — sole known writer). -// * existing undecodable -> write mine, unioned=false (this is MY key — a -// sibling sharing it via copy always writes decodable records, so an undecodable -// value is corruption; overwriting restores correct protection for my holds, and -// the prune side independently ABORTS while an unreadable record is present). +// * existing undecodable -> REMINT (mine, unioned=false, under a fresh key) +// rather than overwriting the corrupt key: overwriting would clear the prune-side +// abort, leaving a same-key sibling's holds unprotected until it republishes. +// Leaving the corrupt key in place keeps the prune-side abort (foldUsageRecords) +// firing so no delete-ward window opens. The sibling writes its own decodable +// record on the next publish tick; the corrupt key is eventually evicted once no +// live instance references it. Narrow gap: a >16 MB value reads back as nullopt +// (indistinguishable from absent), so it takes the fresh-write branch rather than +// remint — both outcomes are safe; the gap is noted in the header's fail-safe list. // * nonce match AND !unioned -> clean replace (sole writer, provably my content; // released holds drop); skipWrite when // byte-identical (idle reload tick). diff --git a/src/usage_scan.cpp b/src/usage_scan.cpp index 53a7bcf..49b91d8 100644 --- a/src/usage_scan.cpp +++ b/src/usage_scan.cpp @@ -69,8 +69,9 @@ using FxParmGetter = // True if any FX in the (possibly container-nested) sub-chain rooted at `fxId` is a // ReaSampler 9000. BOTH fx_ident and original_name are checked on BOTH chain kinds (a -// renamed instance keeps its original_name; fx_ident carries the module path — the -// review's take-path gap is closed by sharing this one walk). Containers are walked via +// renamed instance may keep its original_name; fx_ident carries the module path — the +// primary identification net is the module filename base via fx_ident, which holds even +// after a user renames the FX instance). Containers are walked via // the documented container_count / container_item.X addressing (v7.06+); on a chain // kind or REAPER version without containers the parm read returns empty and recursion // is a no-op. `depth` bounds pathological nesting. fx_ident is queried per FX — chain @@ -83,9 +84,17 @@ bool fxSubtreeHasInstance(const FxParmGetter& parm, int fxId, identityMatches(parm(fxId, "original_name"), id.uidHexUpper, id.nameUpper, id.outputNameUpper)) return true; - if (depth <= 0) return false; const std::string countStr = parm(fxId, "container_count"); - if (countStr.empty()) return false; // not a container + if (countStr.empty()) return false; // not a container; no children to miss + if (depth <= 0) { + // This node IS a container but we have exhausted our descent budget. We cannot + // prove that none of its children is a ReaSampler 9000 instance — treat the + // incomplete walk as a positive identification (the protect direction). This is + // defense-in-depth: kMaxContainerDepth = 32 should prevent reaching this branch + // in any real project, but if it IS reached the fail-safe fires rather than + // silently missing a live nested instance. + return true; + } const int n = std::atoi(countStr.c_str()); for (int k = 0; k < n; ++k) { const std::string item = @@ -98,7 +107,10 @@ bool fxSubtreeHasInstance(const FxParmGetter& parm, int fxId, return false; } -constexpr int kMaxContainerDepth = 8; +// Raised from 8 to 32 (defense in depth against truncation). Real-world FX containers +// are typically 2–4 levels deep; 32 is unreachable in practice while remaining finite. +// Even at 32, the truncation→protect-all guard below is the primary protection. +constexpr int kMaxContainerDepth = 32; std::string trackFxParm(MediaTrack* tr, int fxId, const char* parm) { char buf[2048] = {0}; @@ -183,6 +195,7 @@ UsageScanResult liveInstanceHeldPaths(void* projOpaque) { // undecodable -> the pure fold ABORTS the prune. std::vector usageKeys; { + const std::string prefix = kProjExtUsageKeyPrefix; // hoisted: one alloc, not N char keyBuf[256]; for (int idx = 0;; ++idx) { keyBuf[0] = '\0'; @@ -190,7 +203,6 @@ UsageScanResult liveInstanceHeldPaths(void* projOpaque) { static_cast(sizeof(keyBuf)), nullptr, 0)) break; const std::string key(keyBuf); - const std::string prefix = kProjExtUsageKeyPrefix; if (key.compare(0, prefix.size(), prefix) == 0) usageKeys.push_back(key); } } @@ -198,13 +210,17 @@ UsageScanResult liveInstanceHeldPaths(void* projOpaque) { std::vector> decoded; decoded.reserve(usageKeys.size()); - for (const std::string& key : usageKeys) { + for (std::size_t ki = 0; ki < usageKeys.size(); ++ki) { + const std::string& key = usageKeys[ki]; const std::optional value = readExtStateValue(proj, key.c_str()); if (!value) { decoded.push_back(std::nullopt); // unreadable -> abort (pure fold) + result.offendingKeys.push_back(key); continue; } - decoded.push_back(decodeUsageRecord(*value)); // undecodable -> nullopt -> abort + const std::optional rec = decodeUsageRecord(*value); + if (!rec) result.offendingKeys.push_back(key); + decoded.push_back(rec); // undecodable nullopt -> abort } // 2. Enumerate live ReaSampler 9000 hosts. One channel-frozen needle set drives @@ -250,6 +266,9 @@ UsageScanResult liveInstanceHeldPaths(void* projOpaque) { const UsageFoldResult fold = foldUsageRecords(decoded, liveTrackGuids, anyLive); result.abortPrune = fold.abortPrune; result.heldPaths = fold.heldPaths; + // offendingKeys already populated above (unreadable + undecodable entries); + // clear it on success so callers see it only when abortPrune is set. + if (!result.abortPrune) result.offendingKeys.clear(); return result; } diff --git a/src/usage_scan.h b/src/usage_scan.h index f5df64c..e9fd5d8 100644 --- a/src/usage_scan.h +++ b/src/usage_scan.h @@ -37,12 +37,18 @@ namespace reasampler { // The scan outcome. When abortPrune is true a present rsusage_* record could not be -// read or decoded — the caller MUST halt the prune (delete nothing); heldPaths is then -// meaningless (left empty). Otherwise heldPaths is every project-relative path held by +// read or decoded — the caller MUST halt the prune (delete nothing). offendingKeys +// names the exact "rsusage_" keys that triggered the abort so the action can +// print them for operator recovery (clear via ReaScript: +// reaper.SetProjExtState(0, "reasampler", "", "") +// for each offending key). heldPaths on abort is the protect-all set (every readable +// record's paths) — meaningful only as a belt-and-braces fallback; the abort flag is +// the authoritative signal. Otherwise heldPaths is every project-relative path held by // a live ReaSampler 9000 instance, de-duped, in record order — empty in the common // no-records case (the FX enumeration is skipped entirely). struct UsageScanResult { bool abortPrune = false; + std::vector offendingKeys; // non-empty iff abortPrune std::vector heldPaths; }; diff --git a/tests/test_sample_usage.cpp b/tests/test_sample_usage.cpp index ec4f595..c2d84b0 100644 --- a/tests/test_sample_usage.cpp +++ b/tests/test_sample_usage.cpp @@ -7,15 +7,20 @@ // // Covers: wire round-trip (nonce + unioned flag, empty / adversarial bytes), // malformed -> nullopt, the publish plan's branches (fresh / clean replace + skip / -// sibling union with the sticky poison flag / cross-track re-mint / undecodable heal), +// sibling union with the sticky poison flag / cross-track re-mint / undecodable remint), // the SAME-TRACK SIBLING repro (the review's 🔴#1 — byte-identical wire convergence // must never let one sibling clean-replace the other's still-held paths, including one // write later via the poison flag), the liveness fold (live, dead-track, empty-guid // fallback, de-dup), the ZERO-IDENTIFIED protect-all net (🔴#2 — an identity-matcher // failure must protect everything, not nothing), the UNREADABLE-record abort -// (foldUsageRecords.abortPrune — prune halts, deletes nothing), the pure identity -// matcher (UID hex / module filename base / display name, beta over-protect), and the -// composed pruneOrphans exclusion proof. +// (foldUsageRecords.abortPrune — prune halts, deletes nothing), the UNDECODABLE-EXISTING +// REMINT (corrupt key left in place — prune-side abort keeps firing while sibling holds +// unprotected), the ABORT→PROTECT-ALL belt-and-braces (foldUsageRecords.heldPaths is +// the full protect-all set even when abortPrune is set), the TRUNCATED-WALK→PROTECT-ALL +// proof (FX walk misses a nested instance → anyLive=false → usageHeldPaths protects +// every record — the pure side of the depth-cap + depth-exhaustion-is-container fix), +// the pure identity matcher (UID hex / module filename base / display name, beta +// over-protect), and the composed pruneOrphans exclusion proof. #include "../src/sample_usage.h" @@ -254,16 +259,20 @@ static void testPlanOwnRecordAfterTrackMove() { CHECK(back->trackGuid == "{T2}"); } -static void testPlanUndecodableExisting() { - // An undecodable existing value under MY key is corruption — overwrite with mine - // (the self-heal; the prune side independently aborts while it is unreadable). +static void testPlanUndecodableExistingRemints() { + // An undecodable existing value under MY key must REMINT (not overwrite). Overwriting + // would clear the prune-side abort while a same-key sibling B's holds are unprotected + // until B republishes. Leaving the corrupt key in place keeps the prune-side abort + // (foldUsageRecords.abortPrune) firing so no delete-ward window opens. const UsageRecord mine = makeRecord("{T1}", "NA", {UsageHold{"a", "pa.wav"}}); const UsagePublishPlan plan = planUsagePublish(std::string("corrupt"), mine); - CHECK(!plan.remint); + CHECK(plan.remint); // fresh key — leave the corrupt key untouched CHECK(!plan.skipWrite); + // wire carries mine (to be written under the NEW key by the caller) auto back = decodeUsageRecord(plan.wire); CHECK(back.has_value()); CHECK(back->holds.size() == 1); + CHECK(!back->unioned); // fresh key, sole writer — un-poisoned } // --- liveness fold --------------------------------------------------------------- @@ -370,6 +379,78 @@ static void testUnreadableRecordAbortsPrune() { CHECK(net.heldPaths.size() == 1); } +// --- abort returns the protect-all set (belt-and-braces) --------------------------- +// foldUsageRecords must return heldPaths = EVERY readable record's paths when +// abortPrune is set, so a future caller that forgets to check the flag before using +// heldPaths still gets maximum protection rather than an empty set (which would be +// delete-ward). +static void testAbortFoldReturnsProtectAllSet() { + // Two readable records + one unreadable (nullopt) in between. + std::vector> decoded; + decoded.push_back(makeRecord("{T1}", "N1", {UsageHold{"a", "pa.wav"}})); + decoded.push_back(std::nullopt); // triggers abort + decoded.push_back(makeRecord("{T2}", "N2", {UsageHold{"b", "pb.wav"}})); + + // Live: only {T1} — so without protect-all, T2's path would be excluded. + const UsageFoldResult fold = + foldUsageRecords(decoded, std::unordered_set{"{T1}"}, true); + CHECK(fold.abortPrune); + // heldPaths must contain BOTH paths (protect-all over all readable records), + // not just {T1}'s path. + CHECK(fold.heldPaths.size() == 2); + bool hasPA = false, hasPB = false; + for (const std::string& p : fold.heldPaths) { + if (p == "pa.wav") hasPA = true; + if (p == "pb.wav") hasPB = true; + } + CHECK(hasPA); + CHECK(hasPB); + + // All nullopt (every key unreadable): abort + empty heldPaths (nothing readable). + std::vector> allNull; + allNull.push_back(std::nullopt); + const UsageFoldResult allNullFold = + foldUsageRecords(allNull, std::unordered_set{}, false); + CHECK(allNullFold.abortPrune); + CHECK(allNullFold.heldPaths.empty()); // no readable records to protect +} + +// --- truncated enumeration → protect-all ------------------------------------------- +// The FX walk may truncate at kMaxContainerDepth, leaving a deeply-nested live instance +// missed. At the PURE layer this is indistinguishable from a genuine identity-matcher +// failure: anyInstanceLive stays false while records exist. The protect-all net in +// usageHeldPaths guarantees this resolves toward PROTECT, never toward delete — the same +// test shape as testZeroIdentifiedProtectsAll, stated here explicitly for the truncation +// failure mode. +static void testTruncatedWalkProtectsAll() { + // Records from two tracks that host instances; the FX walk (shell side) failed to + // identify ANY instance (e.g. truncated at depth, or a future matcher gap). + const std::vector records = { + makeRecord("{TRACK-A}", "N1", {UsageHold{"x", "nested-a.wav"}}), + makeRecord("{TRACK-B}", "N2", {UsageHold{"y", "nested-b.wav"}}), + }; + // Shell reported anyLive=false (it couldn't identify any instance — truncated walk). + const std::vector paths = + usageHeldPaths(records, std::unordered_set{}, /*anyInstanceLive=*/false); + // Both paths must be protected — the protect-all net fires. + CHECK(paths.size() == 2); + bool hasA = false, hasB = false; + for (const std::string& p : paths) { + if (p == "nested-a.wav") hasA = true; + if (p == "nested-b.wav") hasB = true; + } + CHECK(hasA); + CHECK(hasB); + + // Belt-and-braces: the same scenario through foldUsageRecords also protects all. + std::vector> decoded; + for (const UsageRecord& r : records) decoded.push_back(r); + const UsageFoldResult fold = + foldUsageRecords(decoded, std::unordered_set{}, false); + CHECK(!fold.abortPrune); + CHECK(fold.heldPaths.size() == 2); +} + // --- the identity matcher ---------------------------------------------------------- // The common-case shapes: REAPER's fx_ident carries the .vst3 MODULE PATH (matched by // the output-name needle "REASAMPLER_9000" — the display name, space-separated, can @@ -472,12 +553,14 @@ int main() { testPlanEmptyNonceNeverClaimsOwnership(); testPlanCrossTrackRemint(); testPlanOwnRecordAfterTrackMove(); - testPlanUndecodableExisting(); + testPlanUndecodableExistingRemints(); testHeldPathsLiveness(); testZeroIdentifiedProtectsAll(); testHeldPathsDedupAndEmptyPathSkip(); testTakeFxAttributedRecordIsProtected(); testUnreadableRecordAbortsPrune(); + testAbortFoldReturnsProtectAllSet(); + testTruncatedWalkProtectsAll(); testIdentityMatcher(); testInstanceHoldMakesPathUnprunable();