fix: classify future-version ledgers with changed record shape correctly, not as corrupt
Version-check now runs on the parseLedger failure path too, so a v3 blob whose record shape actually changed reports FutureVersion instead of Unreadable, avoiding the corrupt-blob "clear it" advice. Also closes the six minor findings.
This commit is contained in:
@@ -141,6 +141,8 @@ bool parseRecordArray(json::Reader& r, OriginLedger& out) {
|
|||||||
|
|
||||||
// `versionOut` stays 0 when no "v" key was present — the legacy path-only shape, or
|
// `versionOut` stays 0 when no "v" key was present — the legacy path-only shape, or
|
||||||
// an empty object. Read regardless of key order, so it is validated after the close.
|
// an empty object. Read regardless of key order, so it is validated after the close.
|
||||||
|
// A duplicate "v" key (never emitted by serialize()) is last-wins, same as every
|
||||||
|
// other duplicate key here — not reachable except by hand-editing the blob.
|
||||||
bool parseLedger(json::Reader& r, OriginLedger& out, int& versionOut) {
|
bool parseLedger(json::Reader& r, OriginLedger& out, int& versionOut) {
|
||||||
if (!r.consume('{')) return false;
|
if (!r.consume('{')) return false;
|
||||||
r.skipWs();
|
r.skipWs();
|
||||||
@@ -174,7 +176,14 @@ ParseOutcome parseStored(const std::string& text, OriginLedger& out) {
|
|||||||
OriginLedger parsed;
|
OriginLedger parsed;
|
||||||
int version = 0;
|
int version = 0;
|
||||||
::reasampler::json::Reader r(text);
|
::reasampler::json::Reader r(text);
|
||||||
if (!parseLedger(r, parsed, version)) return ParseOutcome::Malformed;
|
if (!parseLedger(r, parsed, version)) {
|
||||||
|
// A future record shape (e.g. v3 retyping "kind" or "records") fails these v2
|
||||||
|
// parse rules on the very field the version bump changed. "v" is read before
|
||||||
|
// any record parsing in our own writer's key order, so `version` already holds
|
||||||
|
// it here: report FutureVersion, not Malformed, or the operator gets the
|
||||||
|
// corrupt-blob "clear it" advice against a newer build's ledger.
|
||||||
|
return version > kLedgerVersion ? ParseOutcome::FutureVersion : ParseOutcome::Malformed;
|
||||||
|
}
|
||||||
// Trailing garbage means the blob is not what it claims; accepting it would turn
|
// Trailing garbage means the blob is not what it claims; accepting it would turn
|
||||||
// a detectably-corrupt value into a silently-partial ledger.
|
// a detectably-corrupt value into a silently-partial ledger.
|
||||||
r.skipWs();
|
r.skipWs();
|
||||||
|
|||||||
@@ -50,7 +50,9 @@ void doBankPruneFolder(ReaSamplerSession& session) {
|
|||||||
"ReaSampler than this one, so its records cannot be read safely. It "
|
"ReaSampler than this one, so its records cannot be read safely. It "
|
||||||
"has been left intact and will NOT be overwritten. Reopen the project "
|
"has been left intact and will NOT be overwritten. Reopen the project "
|
||||||
"with that newer version -- do NOT clear this key from here, that "
|
"with that newer version -- do NOT clear this key from here, that "
|
||||||
"would discard tracking records this build cannot see.\n";
|
"would discard tracking records this build cannot see. The block is "
|
||||||
|
"held for the rest of this session, and until then no new capture is "
|
||||||
|
"persisted to the ledger either.\n";
|
||||||
}
|
}
|
||||||
if (!report.unreadableUsageKeys.empty()) {
|
if (!report.unreadableUsageKeys.empty()) {
|
||||||
msg += "One or more instance usage records could not be read or decoded. "
|
msg += "One or more instance usage records could not be read or decoded. "
|
||||||
|
|||||||
@@ -11,6 +11,7 @@
|
|||||||
|
|
||||||
#define REAPERAPI_MINIMAL
|
#define REAPERAPI_MINIMAL
|
||||||
#define REAPERAPI_WANT_EnumProjects
|
#define REAPERAPI_WANT_EnumProjects
|
||||||
|
#define REAPERAPI_WANT_GetProjectName
|
||||||
#define REAPERAPI_WANT_ShowConsoleMsg
|
#define REAPERAPI_WANT_ShowConsoleMsg
|
||||||
#include "reaper_plugin_functions.h"
|
#include "reaper_plugin_functions.h"
|
||||||
|
|
||||||
@@ -64,21 +65,30 @@ void DriveRealtimeCapture(ReaSamplerSession& session)
|
|||||||
// folder, so it survives here untracked — deleting a user's just-recorded
|
// folder, so it survives here untracked — deleting a user's just-recorded
|
||||||
// audio is the destructive direction and is not this shell's call, so the
|
// audio is the destructive direction and is not this shell's call, so the
|
||||||
// path is NAMED instead and the operator decides (docs/TODO.md).
|
// path is NAMED instead and the operator decides (docs/TODO.md).
|
||||||
if (r.status == RealtimeTickStatus::Done && r.result.status == CaptureStatus::Ok)
|
//
|
||||||
|
// RealtimeTickStatus::Done implies CaptureStatus::Ok (RealtimeRecordBackend::
|
||||||
|
// abort only sets Done on that path) — the Ok branch below is therefore the
|
||||||
|
// whole Done case; anything else is Failed and falls to the last branch.
|
||||||
|
if (r.status == RealtimeTickStatus::Done && r.result.status == CaptureStatus::Ok) {
|
||||||
|
// g_rtCaptureProject is still valid here (reset only below) — it names the
|
||||||
|
// ORIGINAL project, not whatever is active now, which is the whole point:
|
||||||
|
// the user has already switched away from it.
|
||||||
|
char nameBuf[512] = {0};
|
||||||
|
GetProjectName(g_rtCaptureProject, nameBuf, sizeof(nameBuf));
|
||||||
|
const std::string projName = nameBuf[0] ? nameBuf : "(unsaved project)";
|
||||||
ShowConsoleMsg(("ReaSampler realtime capture: project switched mid-record -- "
|
ShowConsoleMsg(("ReaSampler realtime capture: project switched mid-record -- "
|
||||||
"captured audio restored into the original project; not "
|
"captured audio restored into the original project (" +
|
||||||
"persisted to avoid crossing projects. The recorded file was "
|
projName + "); not persisted to avoid crossing projects. The "
|
||||||
"left in the original project's bank folder as '" +
|
"recorded file was left in that project's bank folder as '" +
|
||||||
r.result.sample.relativePath +
|
r.result.sample.relativePath +
|
||||||
"', untracked -- reopen that project and re-import it, or "
|
"', untracked. Re-importing it there does NOT adopt this file "
|
||||||
"delete it by hand.\n").c_str());
|
"-- it copies the audio in under a new name -- so after "
|
||||||
else if (r.status == RealtimeTickStatus::Done)
|
"re-importing, delete this untracked original by hand.\n")
|
||||||
ShowConsoleMsg(("ReaSampler realtime capture: project switched mid-record -- "
|
.c_str());
|
||||||
"the capture was aborted and produced no usable file: " +
|
} else {
|
||||||
r.result.message + "\n").c_str());
|
|
||||||
else
|
|
||||||
ShowConsoleMsg(("ReaSampler realtime capture: project changed mid-record -- " +
|
ShowConsoleMsg(("ReaSampler realtime capture: project changed mid-record -- " +
|
||||||
r.result.message + "\n").c_str());
|
r.result.message + "\n").c_str());
|
||||||
|
}
|
||||||
g_rtCapture.reset();
|
g_rtCapture.reset();
|
||||||
g_rtCaptureProject = nullptr;
|
g_rtCaptureProject = nullptr;
|
||||||
return;
|
return;
|
||||||
|
|||||||
@@ -61,9 +61,11 @@ void ReaSamplerSession::recordCreated(const model::Sample& sample,
|
|||||||
const tracking::RecordResult result = tracking_.record(rec);
|
const tracking::RecordResult result = tracking_.record(rec);
|
||||||
// A rejection means a file exists that nothing attributes to us — invisible
|
// A rejection means a file exists that nothing attributes to us — invisible
|
||||||
// otherwise, and exactly the gap this ledger exists to close. AlreadyPresent is
|
// otherwise, and exactly the gap this ledger exists to close. AlreadyPresent is
|
||||||
// the normal dedup outcome, not a gap.
|
// the normal dedup outcome, not a gap. Written as "anything but the two OK
|
||||||
if (result == tracking::RecordResult::RejectedEmptyPath ||
|
// outcomes" rather than a rejection whitelist, so a future RecordResult value
|
||||||
result == tracking::RecordResult::RejectedAbsolutePath) {
|
// is reported by default instead of silently passing through unrecognized.
|
||||||
|
if (result != tracking::RecordResult::Recorded &&
|
||||||
|
result != tracking::RecordResult::AlreadyPresent) {
|
||||||
ShowConsoleMsg(("ReaSampler: could not record the origin of '" +
|
ShowConsoleMsg(("ReaSampler: could not record the origin of '" +
|
||||||
sample.relativePath +
|
sample.relativePath +
|
||||||
"' -- the path is empty or absolute. The file is NOT tracked and "
|
"' -- the path is empty or absolute. The file is NOT tracked and "
|
||||||
|
|||||||
@@ -258,6 +258,17 @@ static void testFutureVersionIsNeitherLoadedNorMalformed() {
|
|||||||
CHECK(loadLedger("{\"v\":2,\"records\":[]}").status == LedgerStatus::Loaded);
|
CHECK(loadLedger("{\"v\":2,\"records\":[]}").status == LedgerStatus::Loaded);
|
||||||
CHECK(loadLedger("{\"v\":1,\"owned\":[\"a.wav\"]}").status == LedgerStatus::Loaded);
|
CHECK(loadLedger("{\"v\":1,\"owned\":[\"a.wav\"]}").status == LedgerStatus::Loaded);
|
||||||
CHECK(loadLedger("{\"v\":-1,\"records\":[]}").status == LedgerStatus::Unreadable);
|
CHECK(loadLedger("{\"v\":-1,\"records\":[]}").status == LedgerStatus::Unreadable);
|
||||||
|
|
||||||
|
// A future version whose record shape actually changed — not just grew — fails
|
||||||
|
// parseRecord under these v2 rules ("kind" retyped from int to string). That must
|
||||||
|
// still classify as FutureVersion, not Unreadable: this is the exact "changed
|
||||||
|
// record shape" case the version ladder comment says v3 means, and Unreadable
|
||||||
|
// would steer the operator into clearing a newer build's ledger.
|
||||||
|
CHECK(loadLedger("{\"v\":3,\"records\":[{\"path\":\"a.wav\",\"kind\":\"loud\"}]}")
|
||||||
|
.status == LedgerStatus::FutureVersion);
|
||||||
|
// Same, with the array itself retyped to an object.
|
||||||
|
CHECK(loadLedger("{\"v\":3,\"records\":{\"path\":\"a.wav\"}}").status ==
|
||||||
|
LedgerStatus::FutureVersion);
|
||||||
}
|
}
|
||||||
|
|
||||||
// A hand-edited or corrupt blob cannot smuggle an absolute or duplicate path past
|
// A hand-edited or corrupt blob cannot smuggle an absolute or duplicate path past
|
||||||
@@ -315,8 +326,11 @@ static void testLegacyPathOnlyManifestLiftsIn() {
|
|||||||
|
|
||||||
// A recapture regenerates the audio behind ONE bank id under a NEW file name
|
// A recapture regenerates the audio behind ONE bank id under a NEW file name
|
||||||
// (makeUniqueTag guarantees it), so the ledger legitimately ends up holding two
|
// (makeUniqueTag guarantees it), so the ledger legitimately ends up holding two
|
||||||
// records with the same sampleId and different paths. The original's lineage survives
|
// records with the same sampleId and different paths. This proves that shape and
|
||||||
// untouched — the Sample was rewritten in place, the birth record was not.
|
// that take-1's record is untouched by take-2's insert (record() never overwrites —
|
||||||
|
// see testLineageIsNeverBackfilled for the same-path case). It does NOT exercise the
|
||||||
|
// ledger-wins-over-the-mutable-Sample claim: nothing here reads Sample.provenance, and
|
||||||
|
// no production code reads OriginRecord.parentSampleId today.
|
||||||
static void testRecaptureAddsSecondRecordUnderTheSameSampleId() {
|
static void testRecaptureAddsSecondRecordUnderTheSameSampleId() {
|
||||||
OriginLedger l;
|
OriginLedger l;
|
||||||
l.record(rec("bank/take-1.wav", OriginKind::Capture, "S-1", "S-parent"));
|
l.record(rec("bank/take-1.wav", OriginKind::Capture, "S-1", "S-parent"));
|
||||||
@@ -326,8 +340,6 @@ static void testRecaptureAddsSecondRecordUnderTheSameSampleId() {
|
|||||||
CHECK(l.size() == 2);
|
CHECK(l.size() == 2);
|
||||||
CHECK(l.find("bank/take-1.wav")->sampleId == "S-1");
|
CHECK(l.find("bank/take-1.wav")->sampleId == "S-1");
|
||||||
CHECK(l.find("bank/take-2.wav")->sampleId == "S-1");
|
CHECK(l.find("bank/take-2.wav")->sampleId == "S-1");
|
||||||
// The ledger, not the (mutable) Sample, is authoritative for lineage: the
|
|
||||||
// recapture's differing parent did not rewrite the original's.
|
|
||||||
CHECK(l.find("bank/take-1.wav")->parentSampleId == "S-parent");
|
CHECK(l.find("bank/take-1.wav")->parentSampleId == "S-parent");
|
||||||
CHECK(l.find("bank/take-1.wav")->kind == OriginKind::Capture);
|
CHECK(l.find("bank/take-1.wav")->kind == OriginKind::Capture);
|
||||||
// Both stay owned, so the superseded file is reclaimable rather than foreign.
|
// Both stay owned, so the superseded file is reclaimable rather than foreign.
|
||||||
|
|||||||
@@ -213,12 +213,16 @@ static void testUnreadableLedgerBlocksAndYieldsNoOrphans() {
|
|||||||
|
|
||||||
// Both blockers at once must both be reported — the operator needs to fix both.
|
// Both blockers at once must both be reported — the operator needs to fix both.
|
||||||
static void testBothBlockersReported() {
|
static void testBothBlockersReported() {
|
||||||
const OriginLedger empty;
|
// Deliberately NON-empty, like testUnreadableLedgerBlocksAndYieldsNoOrphans: an
|
||||||
|
// empty fixture asserts nothing about ownedPaths being withheld, only that it
|
||||||
|
// started empty.
|
||||||
|
OriginLedger populated;
|
||||||
|
populated.record(originOf("bank/would-be-orphan.wav", OriginKind::Capture, "S-1"));
|
||||||
const UsageFoldResult fold = foldLive(
|
const UsageFoldResult fold = foldLive(
|
||||||
{usage("rsusage_A", "{T1}", {UsageHold{"S-held", "bank/held.wav"}}),
|
{usage("rsusage_A", "{T1}", {UsageHold{"S-held", "bank/held.wav"}}),
|
||||||
unreadable("rsusage_BROKEN")},
|
unreadable("rsusage_BROKEN")},
|
||||||
{"{T1}"});
|
{"{T1}"});
|
||||||
const TrackingState state{LedgerStatus::Unreadable, empty, fold};
|
const TrackingState state{LedgerStatus::Unreadable, populated, fold};
|
||||||
const ProtectionAnswer answer = pruneProtection(state);
|
const ProtectionAnswer answer = pruneProtection(state);
|
||||||
CHECK(answer.blocked);
|
CHECK(answer.blocked);
|
||||||
CHECK(answer.ledgerUnreadable);
|
CHECK(answer.ledgerUnreadable);
|
||||||
@@ -227,6 +231,12 @@ static void testBothBlockersReported() {
|
|||||||
// what prune protects, so withholding it would be the unsafe direction.
|
// what prune protects, so withholding it would be the unsafe direction.
|
||||||
CHECK(contains(answer.heldPaths, "bank/held.wav"));
|
CHECK(contains(answer.heldPaths, "bank/held.wav"));
|
||||||
CHECK(answer.ownedPaths.empty());
|
CHECK(answer.ownedPaths.empty());
|
||||||
|
|
||||||
|
// Belt-and-braces: the orphan set computed from this answer is empty even though
|
||||||
|
// the file is present, unreferenced, and recorded as owned.
|
||||||
|
const std::vector<std::string> orphans = reclaim::pruneOrphans(
|
||||||
|
{"bank/would-be-orphan.wav"}, {}, answer.ownedPaths);
|
||||||
|
CHECK(orphans.empty());
|
||||||
}
|
}
|
||||||
|
|
||||||
// A record that exists but whose track hosts no identified instance still protects
|
// A record that exists but whose track hosts no identified instance still protects
|
||||||
|
|||||||
Reference in New Issue
Block a user