Add safe recovery for legacy detector signal schemas - #566
Open
cmargalejo wants to merge 10 commits into
Open
Conversation
This was referenced Jun 12, 2026
for more information, see https://pre-commit.ci
REST files written with TRestDetectorSignal ClassDef < 4 store fSignalTime/fSignalCharge as vector<Float_t>. Since restManager output files do not contain the event-class StreamerInfos, ROOT cannot apply schema evolution and misreads the float payload as doubles, making the detector signal branch unreadable (detectorlib#125). The data is intact on disk and fully recoverable: - macros/legacy/recoverLegacySignalData.C (stage 1, run with plain root): reads the signal branch using replica classes that match the legacy on-disk layout exactly and extracts the data to an intermediate file. Deliberately not named REST_*.C so restRoot's macro loading never interprets the replica class definitions. - macros/legacy/REST_RebuildLegacySignalFile.C (stage 2, run with restRoot): writes a new file with the signal branch rebuilt using the current classes, all other event branches and the AnalysisTree copied, metadata keys preserved, and the event-class StreamerInfos stored this time. Optional in-place overwrite keeps a .bak copy. The on-disk layout is detected via TBranchElement::GetClassVersion() on the fSignal sub-branches; files already at version 4 are skipped. Verified on R00236 (V2.4.0, 261 entries): 19750 signals and 447902 points recovered bit-exact; the rebuilt file reads back correctly via TRestRun with the current vector<Double_t> classes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Legacy DetectorSignal files store split vector<float> payloads while current detectorlib expects vector<double>. Letting ROOT bind that incompatible schema can turn payload bytes into bogus allocation sizes, so recovery must be explicit and conservative. Ordinary TRestRun reads remain nonmutating: this change does not add automatic repair or rewrite behavior. Resolve input and output paths without throwing, reject normalized, symlink, and hard-link aliases, and use ROOT's CREATE-only mode so recovery outputs cannot replace an existing file. In-place recovery refuses pre-existing temporary and .bak paths. Every replacement move is checked; if installing the fixed file fails after backing up the original, a checked rollback restores it when possible and otherwise reports the exact surviving locations. Track unreadable metadata and event branches without dictionaries. A sibling recovery output may still be produced with explicit warnings, but in-place replacement is refused whenever reconstruction omitted known content, leaving the candidate file for inspection and the original untouched. Add focused filesystem tests for path equivalence, existing-output and backup preservation, successful replacement, and fault-injected backup, replacement, and rollback failures. Add an optional canonical-file integration target covering unsafe legacy detection, all-entry reads with the branch disabled, and preservation of the current event after legacy or invalid target selection.
Bind stage-one output to the exact normalized source identity and persist that provenance through the rebuilt result. Reject unknown or inconsistent legacy schemas and malformed flattened signal arrays before creating output. The identity metadata is deliberately an integrity guard for this workflow rather than a cryptographic authentication mechanism. Check every binding, write, flush, and close operation, then reopen rebuilt candidates and validate provenance, tree and branch structure, the current signal schema, and recovered counts before any in-place rename. This keeps the original and any existing backup untouched when validation fails. Add negative coverage for schema, provenance, array corruption, and candidate replacement failures, plus an actual ROOT candidate-readback test.
Expose restRoot --recover-legacy-signals INPUT [--output OUTPUT | --in-place] as an early one-shot command. Orchestrate a build-matched plain ROOT child and a fresh matching REST child without invoking a shell, transport user paths through child-only environment variables, disable startup files, forward signals to the complete child process group, and propagate macro exit statuses. Use canonical authenticated input paths, private mode-0700 same-filesystem work directories, unique candidates, strict complete rebuild validation, and atomic no-replace installation. Keep default sibling output beside the supplied input spelling, preserve basic file permissions for in-place replacement, retain diagnostics on failure, reject existing outputs and dangling symlinks, and never silently overwrite an output or backup created during a race. Preserve every highest-cycle top-level TTree beyond EventTree and AnalysisTree with a fast clone. Validate its key and object names, class, entry count, and recursive branch inventory so an incomplete candidate cannot replace the original. Document the schema transition, workflow, guarantees, and platform limitations. Cover parsing, exact process construction, special-character and symlink paths, environment isolation, executable search permissions, child-group signals and reaping, output races, extra-tree preservation, cleanup and retention, local-only policy, filename suffixes, and in-place replacement behavior. Point the unsafe-branch warning at the new command.
Vindaar
force-pushed
the
cris_legacy_detector_signal_branch
branch
from
August 19, 2026 12:55
e6c8696 to
e73f23f
Compare
cmargalejo
changed the base branch from
cris_fix_event_branch_memory_leak
to
master
August 19, 2026 12:59
Vindaar
approved these changes
Aug 19, 2026
Vindaar
left a comment
Vindaar
left a comment
Member
There was a problem hiding this comment.
With the improvements this PR is now useful to not just warn the user, but also allow reading old files.
Ideally a few people would read all the diff, but given the size of it that is not very realistic. However, please note that the majority of the new added lines are NOT part of the REST core. The majority just deal with the data recovery path that was added. Hence, it's not a new maintenance burden of 4k lines for REST itself (and well, the need for this is our own fault for not making sure we preserve StreamerInfo in the first place).
Vindaar
changed the title
for more information, see https://pre-commit.ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UPDATE 2026-08-19:
This PR changed significantly from what it was initially supposed to do. Instead of just warning if unsupported files are detected, we now have a full blown data recovery feature in REST, which allows us to fix up old files. Essentially when we have an old file that does not yet contain
StreamerInfo(and thus the schema change inTRestRawSignalfromvector<Float_t>tovector<Double_t>of PR rest-for-physics/detectorlib#109 causes a faulty data read leading to an out-of-memory error), we can reinsert that information by rewriting the input file (technically writing a copy) to have thatStreamerInfo, making it readable by current and future REST versions. PR #567 will then handle the case that we don't accidentally lose that information in the future.The large increase in code is mainly because this PR has evolved from a warning-only change into a complete, tested recovery workflow for affected legacy files:
restRoot --recover-legacy-signals INPUT [--output OUTPUT | --in-place]provides a safe one-command recovery path.TRestRunnow detects unsafe explicit reads and points users to the recovery command.The implementation was tested both independently and together with #567, including against the original reproducer. The combined focused suite passes 46/46 tests.
Original PR description
This PR is built on top of #564 and should be reviewed/merged after #564 (I keep it separate because maybe we don't want to change TRestRun.cxx even more just to print a warning).
It mitigates rest-for-physics/detectorlib#125, but it's not a real fix. It just avoids crashing.
Some old files contain
TRestDetectorSignalEventbranches whereTRestDetectorSignal::fSignalTimeandfSignalChargewere written asvector<float>. Current detectorlib expects these members asvector<double>after detectorlib#109.This PR detects that unsupported branch, prints a warning, and disables only that signal event branch. Other events types remain readable.
Update
This PR now also includes the solution, not just the warning:
macros/legacy/recoverLegacySignalData.C(stage 1, plainroot): extracts the legacy float signal data using replica classes matching the old on-disk layout exactly.macros/legacy/REST_RebuildLegacySignalFile.C(stage 2,restRoot): writes a fixed file with the signal branch rebuilt using the current classes, all other branches/metadata copied, and StreamerInfos stored this time. Optional in-place overwrite (keeps a.bak).Depends on #564 (base branch).