Add safe recovery for legacy detector signal schemas - #566
Open
cmargalejo wants to merge 15 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
REST installs every framework macro into every library configuration, and restRoot -m 1 eagerly loads each REST_*.C macro. The legacy stage-two rebuild macro includes TRestDetectorSignalEvent directly, so standalone installations such as axionlib failed macro validation when detectorlib was not built. Exclude the detector-dependent rebuild macro from the generic macro directory install and add it back only when the RestDetector target exists. Keep the detector-independent extraction stage and shared recovery helpers available. If a user invokes the one-command recovery from an installation without detectorlib, fail during preflight with an actionable explanation before creating a work directory or running the extraction stage. Cover the unavailable-detector preflight path and make the racing-output assertion unambiguous to GCC/GoogleTest by adding explicit braces. The latter fixes the Release -Werror=dangling-else failure in the generic CI build.
A renamed/removed class (e.g. TRestRawZeroSupressionToRawProcess) yields a non-null *emulated* TClass built from the file's StreamerInfo, so the old "cl != nullptr" guard passed and key->ReadObj() then aborted inside ROOT: an abstract compiled base (TRestEventProcess) in the emulated object's chain cannot be instantiated, TClass::New fails, and the read segfaults before the "obj == nullptr" skip could run. Skip keys and event branches whose class has no loaded dictionary (HasDictionary()) so they route into the existing skip-and-warn path instead of crashing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmSH2dmqKki9C7akWNigWw
- Default to partial (skip-and-warn) recovery; add --require-complete to opt into strict. The CLI previously hard-coded REQUIRE_COMPLETE=1, refusing exactly the legacy files the tool targets (any file with an obsolete class). In-place recovery still refuses partial content independently. - Accept ROOT's leading launcher flags (restRoot is aliased to "restRoot -l" in thisREST.sh) before --recover-legacy-signals, so the documented command works without bypassing the alias. Updates PrintHelp and the parser tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmSH2dmqKki9C7akWNigWw
for more information, see https://pre-commit.ci
Member
Author
|
Hey @Vindaar, i have played with the recovery on a real file (R00236, 261 entries) and found a couple of things the tests didn't, so I pushed two commits: 1bec438 — segfault fix. Real files have old process metadata whose classes were since renamed/removed (TRestRawZeroSupressionToRawProcess). ROOT builds a non-null emulated TClass for those, so the cl != nullptr guard passed and ReadObj() crashed on an abstract base in the chain. Now I also skip classes with no dictionary (!HasDictionary()) → they go through the normal skip-and-warn path. No fixture had an obsolete class, so nothing caught it. 9492996 — usability. Two things: (1) the CLI forced REQUIRE_COMPLETE=1, so it refused any file with an unreadable class — i.e. the exact files we want to fix. Made partial/skip-and-warn the default, with --require-complete to opt into strict (in-place still refuses partial on its own). (2) It rejected restRoot -l …, but thisREST.sh aliases restRoot to restRoot -l, so the documented command needed \restRoot. Now it accepts the leading -l. Small note: the branch still pins detector at the old v3/float commit, so an as-pinned build fails with "detected version 3." Nothing to do in detectorlib (master's already v4) — just needs the pin ≥ v4 at release. I left the pointer alone and will mention it in the PR description. Give it a look — happy to change anything. |
…option The added assertion passed "input.root" before "--help", but the help shortcut only triggers when --help (or -h) is the single token right after --recover-legacy-signals; with an input present the parser correctly returns kError. Drop the stray input so the case tests "leading launcher flags + --help" as intended. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmSH2dmqKki9C7akWNigWw
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.
Quick summary for reviewers:
This adds a standalone recovery tool for legacy detector-signal files (produced before the vector schema). It's opt-in and doesn't change how REST normally reads or writes files, so it should be low-stress to review.
What it adds (the bulk):
The only framework read-path change is defensive: when REST opens a file whose detector-signal branch uses the old layout, it now detects it, disables that branch, and warns, instead of attempting the read and hitting the >7 GB OOM. That's strictly safer than today's behaviour for anyone who opens an old file, and it's a no-op for normal files.
What it does not change:
How to review quickly: the recovery CLI/macros can be read in isolation; the only thing that affects existing behaviour is the warn-and-disable guard. Easy to revert — it's additive commits, so dropping it leaves the framework exactly as before.
Complements #567: #567 stops REST from producing these files going forward; #566 recovers the ones that already exist.
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).