Reviewing #567
ROOT files can contain historical class layouts and schema-evolution rules needed to read their data correctly. Direct writable opens can let ROOT rewrite that metadata before REST has checked it. This PR centralizes writable opens behind schema preflight, makes file ownership explicit, and validates merged outputs before removing their inputs.
3fec0db1. Every reviewed-file source link points to that exact commit, including newly added files. This page records local test evidence, not live official CI status.Start with the safety boundary and behavior contracts below, then review TRestTools.cxx, run/worker ownership and the dictionary-isolation tests. Deep mode shows the complete feature diff against the pinned master base, with file intent, impact and risk annotations. No knowledge of earlier PR revisions is required.
Exactly what is being reviewed
- Master base
ee5a04f4e662c633e5eaef9f113788431a766cb7- Reviewed #567
3fec0db1b009a3662215127debe901028ea72412· cris_preserve_streamerinfos- Full feature diff
- 43 files; +2630 / −111. SHA-256:
3e994941b12763ed308147d9ea7a6eaadd66954e657ae086f4ade92ee34a7585
Scope exclusions: the published PR also contains validation notes, a benchmark and a workflow replay script under validation/. These support files are excluded from this page's feature inventory and embedded diffs; review them on GitHub if they are to remain in the PR. No submodule gitlink changes are proposed by this implementation.
How the safety boundary works
A missing local UPDATE destination uses ROOT CREATE, preserving ROOT's create-if-missing behavior without overwriting a file that appears during the existence check. Existing files still take the read-only preflight path.
The move-only handle owns the file; Get() and operator-> borrow it. Private exceptions unwind owners before candidate cleanup; public APIs return an invalid handle or false with an error. PrepareBorrowedUpdate bridges legacy callers that already own a READ-mode TFile.
Behavior reviewers must preserve
| Operation | Contract |
|---|---|
| Read / Recreate / Update | Read is permissive about ROOT-supported transports; Recreate intentionally replaces; existing-file Update preflights before writable transition. |
| Merge into an existing target | Pass existingTarget explicitly. Seed the candidate by byte copy. New workers merge together; same-named worker objects replace target objects rather than append to the target tree. Target-only objects stay untouched. |
| Validation | Compare required historical user schemas/rules, recursive key paths/classes and expected tree entry counts. This is not exhaustive value-by-value validation of every arbitrary object; focused tests verify representative payload values. |
| Failure and cleanup | Validate before and after installation; attempt rollback after failed installed validation. Inputs are removed only after success when requested. A cleanup failure can return false even though output is valid: read the error. |
| Compatibility | Keep TRestRun's string-mode API (READ/OPEN and UPDATE); new handle uses an enum. Runs are intentionally non-copyable and not movable. Remote mutation is rejected; choose an explicit local destination. |
Tests and validation evidence
| Revision / scope | Recorded result | Interpretation |
|---|---|---|
| Earlier tested revision c6bdebfd | 78/78 full CTest, serial order; 105/105 local workflow run/build steps | All sublibraries, Geant4/restG4 modern and reference environments, macros, PyROOT and example chains. Local workflow replay, not an official Actions execution. |
| Earlier tested revision c6bdebfd | 26/26 focused non-network tests; pre-commit passed | Host ROOT 6.26/10. Full container run also included the remote-policy test. Modern/reference containers used ROOT 6.26/06 and 6.26/00. |
| Production-code revision 06cddd32 | 26/26 focused non-network tests passed on the final code; configured pre-commit checks passed | Incremental host rebuild and existing focused suite. Later changes concern redundant closes, diagnostics, explicit UPDATE state and an unused parameter; the full all-libraries matrix was not rerun for those edits. |
| Reviewed revision 3fec0db1 · documentation only | Documentation checks and five migration-guide links passed; C++ example compiled and passed create/update/readback smoke testing | No production-code changes since 06cddd32. The standalone example run emitted ROOT build-tree dictionary-location warnings. No full CI rerun is claimed for this documentation update. |
The results above summarize the author's recorded local validation; raw build logs and workflow transcripts are not included in this companion and are not required to use it. They are not independently accessible evidence for readers of this page. Official CI on the final published revision remains the public verification step. Runtime benchmarking showed similar cost, not a demonstrated speedup.
Standalone publication: this single HTML file contains both diffs, annotations, styles, syntax highlighting and interactive controls. No companion files, local checkout, fonts, scripts or network requests are needed to view it. Navigation stays within this page; optional external source links go to GitHub. Repository paths shown in the inventory and code identify reviewed source files, not website dependencies.
Deliberate limits and human-review questions
- This preserves existing usable schema information; it does not reconstruct missing legacy StreamerInfo or correct invalid evolution rules. Legacy recovery remains a separate change.
- ROOT may normalize STL implementation metadata; the exception must stay narrow. Ordinary user/historical identities and embedded rules remain mandatory.
- No interprocess writer lock, universal filesystem atomicity, or power-loss durability is promised. Identity checks are useful but not a lock. Rollback can fail and must report the retained backup.
- Remote reads depend on installed ROOT transports. Remote writes are deliberately refused; Windows runtime behavior has not been validated.
- Confirm installed-result failures unwind and roll back before input cleanup, and check every caller propagates meaningful close/merge errors.
Reviewer checklist
File inventory · full proposed feature
Click a path to open its proposed diff. Risk is reviewer attention, not a claim that a file is defective.
| File | Category / risk / impact | Lines |
|---|---|---|
| CONTRIBUTING.md | developer docsrisk: lowimpact: medium | +11 / −0 |
| README.md | user docsrisk: lowimpact: high | +17 / −2 |
| doc/developer/Safe writable ROOT IO.md | developer docsrisk: lowimpact: medium | +152 / −0 |
| doc/tutorials/Updating ROOT files from macros.md | user docsrisk: lowimpact: high | +87 / −0 |
| macros/REST_AddComponentDataSet.C | call-site adoptionrisk: mediumimpact: medium | +8 / −8 |
| macros/REST_AddComponentFormula.C | call-site adoptionrisk: mediumimpact: medium | +8 / −8 |
| macros/REST_CreateHisto.C | call-site adoptionrisk: mediumimpact: medium | +7 / −2 |
| macros/REST_MergeFiles.C | call-site adoptionrisk: mediumimpact: medium | +7 / −8 |
| source/framework/CMakeLists.txt | buildrisk: lowimpact: medium | +3 / −0 |
| source/framework/analysis/src/TRestDataSetCalibration.cxx | call-site adoptionrisk: mediumimpact: medium | +7 / −2 |
| source/framework/analysis/src/TRestDataSetGainMap.cxx | call-site adoptionrisk: mediumimpact: medium | +17 / −6 |
| source/framework/analysis/src/TRestDataSetOdds.cxx | call-site adoptionrisk: mediumimpact: medium | +7 / −2 |
| source/framework/core/inc/TRestProcessRunner.h | ownershiprisk: highimpact: high | +5 / −3 |
| source/framework/core/inc/TRestRun.h | ownershiprisk: highimpact: high | +8 / −1 |
| source/framework/core/inc/TRestThread.h | ownershiprisk: highimpact: high | +7 / −1 |
| source/framework/core/src/TRestAnalysisTree.cxx | call-site adoptionrisk: mediumimpact: medium | +3 / −2 |
| source/framework/core/src/TRestDataSet.cxx | call-site adoptionrisk: mediumimpact: medium | +6 / −2 |
| source/framework/core/src/TRestDataSetPlot.cxx | call-site adoptionrisk: mediumimpact: medium | +7 / −1 |
| source/framework/core/src/TRestMetadataPlot.cxx | call-site adoptionrisk: mediumimpact: medium | +7 / −3 |
| source/framework/core/src/TRestProcessRunner.cxx | ownershiprisk: highimpact: high | +59 / −18 |
| source/framework/core/src/TRestRun.cxx | ownershiprisk: highimpact: high | +95 / −40 |
| source/framework/core/src/TRestThread.cxx | ownershiprisk: highimpact: high | +22 / −2 |
| source/framework/test/io/CMakeLists.txt | testsrisk: mediumimpact: medium | +121 / −0 |
| source/framework/test/io/ConflictFixture.cxx | testsrisk: mediumimpact: high | +33 / −0 |
| source/framework/test/io/ConflictLinkDef.h | testsrisk: mediumimpact: high | +1 / −0 |
| source/framework/test/io/ConflictPayload.h | testsrisk: mediumimpact: high | +12 / −0 |
| source/framework/test/io/UpdateFixture.cxx | testsrisk: mediumimpact: medium | +37 / −0 |
| source/framework/test/io/UpdateFixtureLoaded.cxx | testsrisk: mediumimpact: medium | +20 / −0 |
| source/framework/test/io/VerifyAnalysisTree.cxx | testsrisk: mediumimpact: medium | +31 / −0 |
| source/framework/test/io/VerifyFixture.cxx | testsrisk: mediumimpact: medium | +43 / −0 |
| source/framework/test/io/VerifyFixtureV1.cxx | testsrisk: mediumimpact: medium | +24 / −0 |
| source/framework/test/io/WriteFixture.cxx | testsrisk: mediumimpact: medium | +27 / −0 |
| source/framework/test/io/WriteSchemaFixture.cxx | testsrisk: mediumimpact: medium | +50 / −0 |
| source/framework/test/io/v1/LinkDef.h | testsrisk: mediumimpact: medium | +4 / −0 |
| source/framework/test/io/v1/TRestIOFixturePayload.cxx | testsrisk: mediumimpact: medium | +3 / −0 |
| source/framework/test/io/v1/TRestIOFixturePayload.h | testsrisk: mediumimpact: medium | +17 / −0 |
| source/framework/test/io/v2/LinkDef.h | testsrisk: mediumimpact: medium | +6 / −0 |
| source/framework/test/io/v2/TRestIOFixturePayload.cxx | testsrisk: mediumimpact: medium | +3 / −0 |
| source/framework/test/io/v2/TRestIOFixturePayload.h | testsrisk: mediumimpact: medium | +18 / −0 |
| source/framework/test/src/RootIO.cxx | testsrisk: mediumimpact: high | +763 / −0 |
| source/framework/test/src/RootIOConflict.cxx | testsrisk: mediumimpact: high | +67 / −0 |
| source/framework/tools/inc/TRestTools.h | ROOT corerisk: highimpact: critical | +69 / −0 |
| source/framework/tools/src/TRestTools.cxx | ROOT corerisk: highimpact: critical | +731 / −0 |
Complete feature diff against master
This is the complete feature diff against the pinned master base, excluding the validation support files noted above. Files and hunks start collapsed. Use the filters to focus on a category or risk level. C++ highlighting is lexical assistance, not a parser.
01CONTRIBUTING.mdFull proposal vs master basedeveloper docsrisk: lowimpact: medium+11 / −0
- Intent
- Document the supported developer entry point, modes, ownership, checked closes, merge behavior and limitations; link it from the contribution guide.
- Invariant
- Preservation is not schema repair, and transactional replacement is not a durability or writer-locking promise.
- Human review
- Verify examples encourage checked Close(), and clarify remote-write rejection and intentionally non-copyable TRestRun.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b47ed6d9..9bfcc4e5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md
Hunk 1@@ -22,6 +22,17 @@ It is recommended that new contributions and functionalities added to REST have
@@ -22,6 +22,17 @@ It is recommended that new contributions and functionalities added to REST have TODO : Explain doxygen formatting, tutorials, where official doc is located. ETC. +### ROOT file I/O changes + +Code that creates, updates, replaces, or merges ROOT files must follow the +[safe writable ROOT I/O guide](doc/developer/Safe%20writable%20ROOT%20IO.md). It documents the required +schema preflight, ownership, transactional replacement, and local/remote path rules. + +This also applies to **existing library code and user-written macros**, not only new code. The handle does not +intercept direct `TFile` calls: audit and migrate remaining UPDATE opens, including constructors and `ReOpen`. +The [user macro migration guide](doc/tutorials/Updating%20ROOT%20files%20from%20macros.md) provides a before/after +example. A passing framework test suite does not establish that external macros have been migrated. + ### Pipeline validation tests TODO : Explain how pipeline validation tests should be implemented
02README.mdFull proposal vs master baseuser docsrisk: lowimpact: high+17 / −2
- Intent
- Make existing user-macro migration visible: warn about direct UPDATE calls, provide a checked-write example, and explain borrowed pointers, close errors and refused updates.
- Invariant
- Installing REST does not intercept direct TFile calls. Preservation is neither legacy recovery nor rollback for an entire macro.
- Human review
- Check that existing private macros are explicitly in scope, read-only macros are distinguished, and the example never bypasses failed preflight.
diff --git a/README.md b/README.md index d871de18..b8f90a36 100644 --- a/README.md +++ b/README.md
Hunk 1@@ -8,6 +8,19 @@
@@ -8,6 +8,19 @@ The REST-for-Physics (Rare Event Searches Toolkit) Framework is mainly written in C++ and it is fully integrated with [ROOT](https://root.cern.ch) I/O interface. REST was initially born as a collaborative software effort to provide common tools for acquisition, simulation, and data analysis of gaseous Time Projection Chambers (TPCs). However, the framework is already extending its usage to be non-exclusive of detector data analysis. The possibilities of the framework are provided by the different libraries and packages written for REST in our community. +## Important: review existing macros that update ROOT files + +If your macro adds histograms, changes metadata, or otherwise modifies an existing ROOT file, replace direct +`TFile` UPDATE opens with `TRestRootFileHandle`. This applies to **existing user macros and library code**, not +just new development. Direct ROOT calls are not automatically protected by REST and can still lose historical +schema information, even when the macro only writes a histogram. Read-only macros do not need this migration. + +See [Updating ROOT files from macros](doc/tutorials/Updating%20ROOT%20files%20from%20macros.md) for a migration +example, ownership rules, and what to do if an update is refused. The new interface addresses an existing risk; +it does not make previously written macros newly unsafe. + +## Framework overview + The REST Framework provides 3 interfaces that prototype the use of **event types**, **metadata** and **event processes** through `TRestEvent`, `TRestMetadata` and `TRestEventProcess` abstract class definitions. Any REST library will implement **specific objects** that inherit from those 3 basic interfaces.
Hunk 2@@ -63,8 +76,10 @@ Any **metadata** object written with REST **will be stamped** with few metadata
@@ -63,8 +76,10 @@ Any **metadata** object written with REST **will be stamped** with few metadata If different REST versions were used to write a ROOT file, e.g. at different steps of the data processing chain, the historic metadata objects will preserve their original version. However, the `TRestRun` metadata object **will always store** the version used to write the ROOT file. -After REST release 2.2.1., REST implements correctly the `ROOT schema evolution`. Therefore, any new REST version should always be backwards compatible. -I.e. Any file written after v2.2.1 should be readable without problems with any future version. +REST uses ROOT schema evolution to support reading historical data. Compatibility depends on usable historical +schema information and compatible class definitions/evolution rules; it is not guaranteed by a file's release +number alone. Use the checked writable interface described above to preserve existing schema information. +Files already missing required information may need separate recovery. A major change at 2.3 will prevent from backwards compatibility, since class names have been reviewed.
03doc/developer/Safe writable ROOT IO.mdFull proposal vs master basedeveloper docsrisk: lowimpact: medium+152 / −0
- Intent
- Document the supported developer entry point, modes, ownership, checked closes, merge behavior and limitations; link it from the contribution guide.
- Invariant
- Preservation is not schema repair, and transactional replacement is not a durability or writer-locking promise.
- Human review
- Verify examples encourage checked Close(), and clarify remote-write rejection and intentionally non-copyable TRestRun.
diff --git a/doc/developer/Safe writable ROOT IO.md b/doc/developer/Safe writable ROOT IO.md new file mode 100644 index 00000000..5067729c --- /dev/null +++ b/doc/developer/Safe writable ROOT IO.md
Hunk 1@@ -0,0 +1,152 @@
@@ -0,0 +1,152 @@
+# Safe writable ROOT I/O
+
+REST ROOT files may contain several historical `TStreamerInfo` entries and embedded schema rules. Opening such
+a file directly with `TFile::Open(..., "UPDATE")` bypasses REST's schema preflight and can let ROOT rewrite
+schema metadata before REST has established that every historical class layout is usable. Framework code that
+creates or mutates ROOT files must therefore use `TRestRootFileHandle`.
+
+## Existing code and user macros need attention too
+
+**This is not only a convention for new code.** Existing framework/library code and user-written macros that
+open ROOT files directly for UPDATE still bypass the protection. The handle does not intercept `TFile::Open`,
+`TFile` constructors, or `ReOpen`; installing a newer REST version does not redirect those calls automatically.
+Even an update that only adds a histogram can rewrite file-level schema metadata. Whether information is lost
+depends on the file's historical schemas and ROOT's handling of them; this change addresses a pre-existing risk.
+
+Audit existing writable opens and migrate them to the handle, or use the borrowed-file adapter below where
+ownership cannot be changed. The adapter must receive a READ-mode file, not one already opened for UPDATE.
+Read-only opens do not need migration for this schema-preservation protection. See the
+[user-facing migration example](../tutorials/Updating%20ROOT%20files%20from%20macros.md), which also covers error
+handling, borrowed pointers, and existing-file safety. Do not assume all library or external macros are covered
+merely because framework CI passes.
+
+## Opening files
+
+Use `TRestRootFileHandle::Open` for new code and when migrating existing writable opens:
+
+```cpp
+#include "TRestTools.h"
+
+auto file = TRestRootFileHandle::Open(filename, TRestRootFileMode::Update);
+if (!file) {
+ ReportError(file.Error());
+ return false;
+}
+
+file->cd();
+WriteObjects();
+
+if (!file.Close()) {
+ ReportError(file.Error());
+ return false;
+}
+```
+
+The available modes are `Read`, `Recreate`, and `Update`. `Recreate` intentionally replaces an existing file
+and must not be used as a shortcut for `Update`. Like ROOT's UPDATE mode, `Update` creates a missing local
+file; REST uses CREATE for this case so that a concurrently appearing file cannot be overwritten.
+An existing file is initially opened read-only. REST inventories the
+exact class-name, class-version, and checksum tuples stored in the file, collects the embedded schema rules,
+asks ROOT to resolve the on-disk entries into loaded or emulated classes, and only then registers the embedded
+rules. Inventory and resolution share one read of the on-disk schema record. REST checks that each resolved
+user schema still has its original name, version, and checksum; a conflicting cached layout is rejected before
+the file becomes writable. This follows ROOT's own `TFile::ReadStreamerInfo`/`TStreamerInfo::BuildCheck` ownership and resolution
+rules, including unloaded classes and entries such as `ROOT::TIOFeatures`. REST then transitions the same
+`TFile` to update mode and checks that its filesystem identity still matches the identity captured before
+preflight. It then marks the historical class-index entries required for writing.
+
+`PrepareBorrowedUpdate(TFile&, std::string*)` provides the same update preparation when legacy code already
+owns a `TFile`. The supplied file must be valid, open in `READ` mode, and not writable:
+
+```cpp
+std::unique_ptr<TFile> file(TFile::Open(filename.c_str(), "READ"));
+std::string error;
+if (!file || !TRestRootFileHandle::PrepareBorrowedUpdate(*file, &error)) {
+ ReportError(error);
+ return false;
+}
+```
+
+Prefer `TRestRootFileHandle` whenever ownership can be changed. A borrowed file remains the caller's
+responsibility, including checking its close/write status. If preparation fails, propagate the error and do not
+attempt to write through that file.
+
+The preflight preserves semantically required historical user StreamerInfos and schema rules; it does not make
+an incompatible class change or an incorrect schema rule valid. ROOT may normalize or omit generated
+standard-library implementation metadata (for example libstdc++ `__pair_base` descriptors) when writing a
+file. REST accepts that ROOT-defined normalization but still requires exact identities for ordinary user and
+historical class schemas, plus every embedded rule. Class authors must still increment class versions as
+required, write correct evolution rules, and test representative old files both before and after a writable
+open.
+
+## Ownership and error handling
+
+`TRestRootFileHandle` is move-only. Pass it by reference while it remains owned by a component, or transfer it
+with `std::move`. Pointers returned by `Get()` and `operator->` are non-owning and must not be deleted or retained
+beyond the handle's lifetime.
+
+The destructor closes an open file, but cannot report failure. Code that writes must call `Close()` explicitly
+and handle a false result using `Error()`. Close a live destination before move-assigning another handle to it,
+because move assignment cannot return a close error for the previous file.
+
+## Replacing or merging files
+
+Use `TRestTools::MergeRootFilesTransactionally` instead of merging directly into the destination or manually
+renaming a partially written file:
+
+```cpp
+std::string error;
+const std::string existing = outputAlreadyExists ? output : "";
+if (!TRestTools::MergeRootFilesTransactionally(output, newInputs, existing, true, &error)) {
+ ReportError(error);
+ return false;
+}
+```
+
+When non-empty, `existingTarget` is copied byte-for-byte to the same-directory temporary file and opened through
+the checked UPDATE path. Only `newInputs` are passed to ROOT's merger. This deliberately preserves ROOT's
+historical UPDATE behavior: target-only objects are not deserialized or rewritten, while a same-named object
+from the new inputs replaces the old target object. Callers updating an existing output must pass it explicitly;
+otherwise its existing contents are not part of the merge.
+
+The helper inventories every input, rejects incompatible classes at the same key path, and constructs the
+result in a temporary sibling of the local destination. Before replacement it validates the expected user
+StreamerInfos and schema rules, recursive key paths and classes, and `TTree` entry counts using ROOT's UPDATE
+semantics (summed across new inputs, replacing a same-named target tree). It validates the installed file again
+and attempts to restore the previous destination from a rollback backup on failure.
+Destination paths and cleanup exclusions resolve filesystem aliases, so using `./` or a symbolic link cannot
+cause input cleanup to delete the completed output. Local input files are removed only after successful replacement and validation when
+`removeInputsOnSuccess` is true.
+
+A false return can also mean that the merged output is valid but a backup or input could not be removed.
+Always inspect the returned error before deciding how to recover. Replacement uses platform filesystem
+operations on sibling paths, but this is not a promise of power-loss durability or atomic behavior on every
+mounted filesystem. Rollback can itself fail; preserve and report the detailed error, including any retained
+backup path.
+
+Internally, the update and merge operations translate failures into exceptions to unwind ROOT file owners
+before cleanup. Their public interfaces retain boolean results and error strings. Both temporary and installed
+merge results pass the same schema/content validator, and any exception during installed-result validation
+triggers rollback before the failure is returned.
+
+## Local and remote paths
+
+Remote ROOT files may be read and may be merge inputs if the installed ROOT transports can open them. Writable
+opens and transactional merge destinations must resolve to local paths; remote URLs are rejected before
+mutation. Local `file://` URLs are accepted. Use `TRestTools::IsRemoteRootPath` when a caller needs to validate
+or explain this policy before opening a file.
+
+Only local entries in `inputFiles` are candidates for removal after a successful merge. Remote inputs are not
+deleted by the helper.
+
+## `TRestRun` is intentionally non-copyable
+
+`TRestRun` owns live input and output handles and also holds raw aliases to file-owned objects. The previous
+implicit copy would have shallow-copied that state, making ownership and lifetime unsafe. Its copy constructor
+and copy assignment operator are therefore deleted.
+
+APIs should pass runs as `TRestRun&`, `const TRestRun&`, or pointers rather than by value. Use
+`std::unique_ptr<TRestRun>` when ownership of a run object itself must be transferred, or construct a separate
+`TRestRun` from the appropriate filename/configuration when an independent instance is required. `TRestRun`
+does not currently expose move construction or move assignment, so do not rely on `std::move` to transfer the
+object directly.
04doc/tutorials/Updating ROOT files from macros.mdFull proposal vs master baseuser docsrisk: lowimpact: high+87 / −0
- Intent
- Make existing user-macro migration visible: warn about direct UPDATE calls, provide a checked-write example, and explain borrowed pointers, close errors and refused updates.
- Invariant
- Installing REST does not intercept direct TFile calls. Preservation is neither legacy recovery nor rollback for an entire macro.
- Human review
- Check that existing private macros are explicitly in scope, read-only macros are distinguished, and the example never bypasses failed preflight.
diff --git a/doc/tutorials/Updating ROOT files from macros.md b/doc/tutorials/Updating ROOT files from macros.md new file mode 100644 index 00000000..b5c4cb96 --- /dev/null +++ b/doc/tutorials/Updating ROOT files from macros.md
Hunk 1@@ -0,0 +1,87 @@
@@ -0,0 +1,87 @@
+# Updating ROOT files from macros
+
+**Review your existing macros as well as new ones.** If a macro adds histograms, writes a readout, changes
+metadata, or otherwise modifies an existing ROOT file, use `TRestRootFileHandle` instead of a direct `TFile`
+UPDATE open. This applies to personal macros outside the REST repositories too.
+
+ROOT files can contain historical class descriptions (`StreamerInfo`) and schema-evolution rules needed to
+read their data. A direct UPDATE open can allow ROOT to rewrite this metadata without REST's preservation
+checks, even if your macro only adds a histogram. Not every UPDATE loses information, but successful execution
+alone does not prove historical data remains readable.
+
+This is a pre-existing risk, not a new incompatibility introduced by the handle. Updating REST protects code
+that uses the checked interface; it does **not** automatically redirect direct ROOT calls in your macros.
+Macros that only read files do not need this migration for schema preservation.
+
+## What to look for
+
+Look for `TFile::Open(..., "UPDATE")`, stack/heap `TFile` constructors using UPDATE (including lowercase
+`"update"`), and calls to `ReOpen("UPDATE")`. Check Python/PyROOT macros and helper functions too. For example:
+
+```sh
+rg -n -i 'update|reopen' --glob '*.{C,cxx,cpp,h,py}' path/to/your/macros
+```
+
+This is only a starting point: inspect mode variables and wrapper functions manually. A search or CI check is
+not proof that every writable open is safe, and repository CI cannot inspect private macros on your machine.
+
+## Replace the open, preserve the write, check the close
+
+The old pattern bypasses REST's preflight:
+
+```cpp
+TFile* file = TFile::Open(filename, "UPDATE");
+// Write histograms, metadata, etc.
+file->Close();
+delete file;
+```
+
+Use a REST build that provides `TRestRootFileHandle`, with REST loaded in your macro environment. This complete
+example adds a small metadata note; use the same structure around your own histogram or metadata writes:
+
+```cpp
+#include <TFile.h>
+#include <TNamed.h>
+#include <iostream>
+
+#include "TRestTools.h"
+
+bool AddAnalysisNote(const char* filename) {
+ auto file = TRestRootFileHandle::Open(filename, TRestRootFileMode::Update);
+ if (!file) {
+ std::cerr << file.Error() << '\n';
+ return false;
+ }
+
+ file->cd();
+ TNamed note("analysisNote", "Updated with checked REST ROOT I/O");
+ const bool written = note.Write() > 0;
+ const bool closed = file.Close();
+ if (!written) std::cerr << "Could not write analysisNote\n";
+ if (!closed) std::cerr << file.Error() << '\n';
+ return written && closed;
+}
+```
+
+The handle owns the file and closes it automatically on destruction, but writers must explicitly check
+`Close()` to report errors. Pass `file.Get()` to APIs expecting a `TFile*`; that pointer is borrowed. Do not
+delete it or use it, or file-owned objects, after the handle closes. Do not retain the old `delete file` line.
+
+Like ROOT UPDATE, `Update` creates a missing local file. If your macro requires an existing input, keep that
+existence check. `Recreate` intentionally replaces existing contents: never substitute it for an UPDATE that
+failed. Writable destinations must be local; remote reads remain subject to ROOT's available transports.
+
+## If the update is refused
+
+Report the error and stop; do not fall back to direct ROOT UPDATE or change the mode to RECREATE. Consult the
+error before deciding whether a compatible dictionary, corrected schema rule, or separate legacy recovery is
+needed. The handle preserves usable schema information already present; it does not reconstruct missing
+StreamerInfo or repair incompatible evolution rules.
+
+Keep a backup before modifying valuable data. Checked UPDATE is not a rollback transaction for your entire
+macro: failed writes may leave partial changes, and it does not provide concurrent-writer locking or power-loss
+durability. To work on a remote file, explicitly create a local copy and update that copy.
+
+For existing code that must retain ownership of a READ-mode `TFile`, see `PrepareBorrowedUpdate` in the
+[developer guide](../developer/Safe%20writable%20ROOT%20IO.md). The same guide covers transactional merging and
+the intentionally non-copyable `TRestRun` interface.
05macros/REST_AddComponentDataSet.CFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+8 / −8
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/macros/REST_AddComponentDataSet.C b/macros/REST_AddComponentDataSet.C index f0c60044..ff508359 100644 --- a/macros/REST_AddComponentDataSet.C +++ b/macros/REST_AddComponentDataSet.C
Hunk 1@@ -1,5 +1,6 @@
@@ -1,5 +1,6 @@ #include "TRestComponent.h" #include "TRestTask.h" +#include "TRestTools.h" #ifndef RestTask_AddComponent #define RestTask_AddComponent
Hunk 2@@ -29,18 +30,17 @@ Int_t REST_AddComponentDataSet(std::string cfgFile, std::string sectionName,
@@ -29,18 +30,17 @@ Int_t REST_AddComponentDataSet(std::string cfgFile, std::string sectionName,
TRestComponentDataSet comp(cfgFile.c_str(), sectionName.c_str());
comp.Initialize();
- TFile* f;
- if (update)
- f = TFile::Open(outputFile.c_str(), "UPDATE");
- else
- f = TFile::Open(outputFile.c_str(), "RECREATE");
+ auto file = TRestRootFileHandle::Open(outputFile,
+ update ? TRestRootFileMode::Update : TRestRootFileMode::Recreate);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return -1;
+ }
if (componentName == "") componentName = sectionName;
comp.Write(componentName.c_str());
- f->Close();
-
- return 0;
+ return file.Close() ? 0 : -1;
}
#endif
06macros/REST_AddComponentFormula.CFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+8 / −8
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/macros/REST_AddComponentFormula.C b/macros/REST_AddComponentFormula.C index 940866c3..7c7f045d 100644 --- a/macros/REST_AddComponentFormula.C +++ b/macros/REST_AddComponentFormula.C
Hunk 1@@ -1,5 +1,6 @@
@@ -1,5 +1,6 @@ #include "TRestComponent.h" #include "TRestTask.h" +#include "TRestTools.h" #ifndef RestTask_AddComponentFormula #define RestTask_AddComponentFormula
Hunk 2@@ -29,18 +30,17 @@ Int_t REST_AddComponentFormula(std::string cfgFile, std::string sectionName,
@@ -29,18 +30,17 @@ Int_t REST_AddComponentFormula(std::string cfgFile, std::string sectionName,
TRestComponentFormula comp(cfgFile.c_str(), sectionName.c_str());
comp.Initialize();
- TFile* f;
- if (update)
- f = TFile::Open(outputFile.c_str(), "UPDATE");
- else
- f = TFile::Open(outputFile.c_str(), "RECREATE");
+ auto file = TRestRootFileHandle::Open(outputFile,
+ update ? TRestRootFileMode::Update : TRestRootFileMode::Recreate);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return -1;
+ }
if (componentName == "") componentName = sectionName;
comp.Write(componentName.c_str());
- f->Close();
-
- return 0;
+ return file.Close() ? 0 : -1;
}
#endif
07macros/REST_CreateHisto.CFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+7 / −2
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/macros/REST_CreateHisto.C b/macros/REST_CreateHisto.C index 91159d22..16b84e78 100644 --- a/macros/REST_CreateHisto.C +++ b/macros/REST_CreateHisto.C
Hunk 1@@ -5,6 +5,7 @@
@@ -5,6 +5,7 @@ #include <TMath.h> #include <TRestRun.h> #include <TRestTask.h> +#include <TRestTools.h> #include <TSystem.h> #ifndef RestTask_CreateHisto
Hunk 2@@ -54,9 +55,13 @@ Int_t REST_CreateHisto(string varName, string rootFileName, TString histoName, i
@@ -54,9 +55,13 @@ Int_t REST_CreateHisto(string varName, string rootFileName, TString histoName, i
h->Scale(normFactor);
- TFile* f = new TFile((TString)rootFileName, "update");
+ auto file = TRestRootFileHandle::Open(rootFileName, TRestRootFileMode::Update);
+ if (!file) {
+ RESTLog << file.Error() << RESTendl;
+ return -1;
+ }
h->Write(histoName);
- f->Close();
+ if (!file.Close()) return -1;
RESTLog << "Written histogram " << histoName << " into " << rootFileName << RESTendl;
08macros/REST_MergeFiles.CFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+7 / −8
- Intent
- Delegate macro merging to the transactional helper and report failures.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/macros/REST_MergeFiles.C b/macros/REST_MergeFiles.C index 3d9bd487..f9c2a8bb 100644 --- a/macros/REST_MergeFiles.C +++ b/macros/REST_MergeFiles.C
Hunk 1@@ -1,5 +1,5 @@
@@ -1,5 +1,5 @@ -#include "TFileMerger.h" #include "TRestTask.h" +#include "TRestTools.h" #ifndef RESTTask_MergeFiles #define RESTTask_MergeFiles
Hunk 2@@ -12,14 +12,13 @@
@@ -12,14 +12,13 @@
//*******************************************************************************************************
Int_t REST_MergeFiles(TString pathAndPattern, TString outputFilename) {
vector<string> files = TRestTools::GetFilesMatchingPattern((string)pathAndPattern);
- TFileMerger* m = new TFileMerger(false);
- m->OutputFile(outputFilename);
- for (auto f : files) {
- m->AddFile(f.c_str());
+ std::string error;
+ const bool success =
+ TRestTools::MergeRootFilesTransactionally(outputFilename.Data(), files, "", false, &error);
+ if (!success) {
+ RESTError << error << RESTendl;
}
- int a = m->Merge();
- delete m;
- return a;
+ return success;
// TRestRunMerger *runMerger = new TRestRunMerger( pathAndPattern );
09source/framework/CMakeLists.txtFull proposal vs master basebuildrisk: lowimpact: medium+3 / −0
- Intent
- Wire ROOT I/O fixture building into the existing framework test configuration.
- Invariant
- Tests remain part of normal CMake/CTest, not a separate required testing framework.
- Human review
- Check the test-enabled condition and target dependency ordering.
diff --git a/source/framework/CMakeLists.txt b/source/framework/CMakeLists.txt index b6440e4d..98ca9e2f 100644 --- a/source/framework/CMakeLists.txt +++ b/source/framework/CMakeLists.txt
Hunk 1@@ -27,3 +27,6 @@ endif (CMAKE_SYSTEM_NAME MATCHES "Windows")
@@ -27,3 +27,6 @@ endif (CMAKE_SYSTEM_NAME MATCHES "Windows") compiledir(RestFramework) add_library_test() +if (TEST) + add_subdirectory(test/io) +endif ()
10source/framework/analysis/src/TRestDataSetCalibration.cxxFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+7 / −2
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/source/framework/analysis/src/TRestDataSetCalibration.cxx b/source/framework/analysis/src/TRestDataSetCalibration.cxx index f6794f3a..1f10e638 100644 --- a/source/framework/analysis/src/TRestDataSetCalibration.cxx +++ b/source/framework/analysis/src/TRestDataSetCalibration.cxx
Hunk 1@@ -100,6 +100,7 @@
@@ -100,6 +100,7 @@ #include "TRestDataSetCalibration.h" #include "TRestDataSet.h" +#include "TRestTools.h" ClassImp(TRestDataSetCalibration);
Hunk 2@@ -257,14 +258,18 @@ void TRestDataSetCalibration::Calibrate() {
@@ -257,14 +258,18 @@ void TRestDataSetCalibration::Calibrate() {
if (!fOutputFileName.empty()) {
if (TRestTools::GetFileNameExtension(fOutputFileName) == "root") {
dataSet.Export(fOutputFileName);
- TFile* f = TFile::Open(fOutputFileName.c_str(), "UPDATE");
+ auto file = TRestRootFileHandle::Open(fOutputFileName, TRestRootFileMode::Update);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
this->Write();
if (gr) gr->Write();
if (linearFit) linearFit->Write();
// if(lFit)lFit->Write();
// spectrumFit->Write();
if (spectrum) spectrum->Write();
- f->Close();
+ if (!file.Close()) RESTError << file.Error() << RESTendl;
}
}
}
11source/framework/analysis/src/TRestDataSetGainMap.cxxFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+17 / −6
- Intent
- Adopt checked ownership for gain-map file operations; UPDATE must retain create-if-missing export behavior.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/source/framework/analysis/src/TRestDataSetGainMap.cxx b/source/framework/analysis/src/TRestDataSetGainMap.cxx index ef5d53b4..65fa5511 100644 --- a/source/framework/analysis/src/TRestDataSetGainMap.cxx +++ b/source/framework/analysis/src/TRestDataSetGainMap.cxx
Hunk 1@@ -139,6 +139,8 @@
@@ -139,6 +139,8 @@ #include "TRestDataSetGainMap.h" +#include "TRestTools.h" + ClassImp(TRestDataSetGainMap); /////////////////////////////////////////////// /// \brief Default constructor
Hunk 2@@ -320,10 +322,13 @@ void TRestDataSetGainMap::CalibrateDataSet(const std::string& dataSetFileName, s
@@ -320,10 +322,13 @@ void TRestDataSetGainMap::CalibrateDataSet(const std::string& dataSetFileName, s
dataSet.Export(outputFileName, std::vector<std::string>(excludeCol.begin(), excludeCol.end()));
// Add this TRestDataSetGainMap metadata to the output file
- TFile* f = TFile::Open(outputFileName.c_str(), "UPDATE");
+ auto file = TRestRootFileHandle::Open(outputFileName, TRestRootFileMode::Update);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
this->Write();
- f->Close();
- delete f;
+ if (!file.Close()) RESTError << file.Error() << RESTendl;
}
/////////////////////////////////////////////
Hunk 3@@ -505,10 +510,16 @@ void TRestDataSetGainMap::Export(const std::string& fileName) {
@@ -505,10 +510,16 @@ void TRestDataSetGainMap::Export(const std::string& fileName) {
}
if (TRestTools::GetFileNameExtension(fOutputFileName) == "root") {
- TFile* f = TFile::Open(fOutputFileName.c_str(), "UPDATE");
+ auto file = TRestRootFileHandle::Open(fOutputFileName, TRestRootFileMode::Update);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
this->Write(GetName());
- f->Close();
- delete f;
+ if (!file.Close()) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
RESTInfo << "Calibration saved to " << fOutputFileName << RESTendl;
} else
RESTError << "File extension for " << fOutputFileName << "is not supported." << RESTendl;
12source/framework/analysis/src/TRestDataSetOdds.cxxFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+7 / −2
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/source/framework/analysis/src/TRestDataSetOdds.cxx b/source/framework/analysis/src/TRestDataSetOdds.cxx index 594c82e5..921338e5 100644 --- a/source/framework/analysis/src/TRestDataSetOdds.cxx +++ b/source/framework/analysis/src/TRestDataSetOdds.cxx
Hunk 1@@ -104,6 +104,7 @@
@@ -104,6 +104,7 @@ #include "TRestDataSetOdds.h" #include "TRestDataSet.h" +#include "TRestTools.h" ClassImp(TRestDataSetOdds);
Hunk 2@@ -273,11 +274,15 @@ void TRestDataSetOdds::ComputeLogOdds() {
@@ -273,11 +274,15 @@ void TRestDataSetOdds::ComputeLogOdds() {
if (TRestTools::GetFileNameExtension(fOutputFileName) == "root") {
RESTDebug << "Exporting dataset to " << fOutputFileName << RESTendl;
dataSet.Export(fOutputFileName);
- TFile* f = TFile::Open(fOutputFileName.c_str(), "UPDATE");
+ auto file = TRestRootFileHandle::Open(fOutputFileName, TRestRootFileMode::Update);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
this->Write();
RESTDebug << "Writing histograms to " << fOutputFileName << RESTendl;
for (const auto& [obsName, histo] : fHistos) histo->Write();
- f->Close();
+ if (!file.Close()) RESTError << file.Error() << RESTendl;
}
}
}
13source/framework/core/inc/TRestProcessRunner.hFull proposal vs master baseownershiprisk: highimpact: high+5 / −3
- Intent
- Make worker-file ownership explicit and hand completed worker outputs to the centralized merge path.
- Invariant
- A file has one owner; worker writes must finish and close before merge or cleanup.
- Human review
- Check multi-worker teardown and failure propagation, not just the mechanical Open substitution.
diff --git a/source/framework/core/inc/TRestProcessRunner.h b/source/framework/core/inc/TRestProcessRunner.h index bb8ef5ab..2809d6a8 100644 --- a/source/framework/core/inc/TRestProcessRunner.h +++ b/source/framework/core/inc/TRestProcessRunner.h
Hunk 1@@ -9,6 +9,7 @@
@@ -9,6 +9,7 @@ #include "TRestEventProcess.h" #include "TRestMetadata.h" #include "TRestRun.h" +#include "TRestTools.h" #define TIME_MEASUREMENT
Hunk 2@@ -35,8 +36,9 @@ class TRestProcessRunner : public TRestMetadata {
@@ -35,8 +36,9 @@ class TRestProcessRunner : public TRestMetadata {
TRestEvent* fOutputEvent; //!
// self variables for processing
- std::vector<TRestThread*> fThreads; //!
- TFile* fOutputDataFile; //! the TFile pointer being used
+ std::vector<TRestThread*> fThreads; //!
+ TRestRootFileHandle fOutputDataFileOwner; //!
+ TFile* fOutputDataFile; //! the TFile pointer being used
TString fOutputDataFileName; //! indicates the name of the first file created as output data file. The
//! actual output file maybe changed if tree is too large
TTree* fEventTree; //!
Hunk 3@@ -99,7 +101,7 @@ class TRestProcessRunner : public TRestMetadata {
@@ -99,7 +101,7 @@ class TRestProcessRunner : public TRestMetadata {
void FillThreadEventFunc(TRestThread* t);
void ConfigOutputFile();
void MergeOutputFile();
- void WriteProcessesMetadata();
+ void WriteProcessesMetadata(TFile* destination = nullptr);
// tools
void ResetRunTimes();
14source/framework/core/inc/TRestRun.hFull proposal vs master baseownershiprisk: highimpact: high+8 / −1
- Intent
- Give TRestRun owned input/output handles while retaining raw aliases and its legacy string-mode entry point; route worker merging through the transaction helper.
- Invariant
- No shallow copies of live run ownership. CloseFile clears owners before the replacement paths; redundant later closes are removed.
- Human review
- Follow tree/file destruction order, split-file processing and post-merge metadata writes. Existing macros must not start owning borrowed pointers.
diff --git a/source/framework/core/inc/TRestRun.h b/source/framework/core/inc/TRestRun.h index a830bd6a..6e693cf4 100644 --- a/source/framework/core/inc/TRestRun.h +++ b/source/framework/core/inc/TRestRun.h
Hunk 1@@ -5,12 +5,12 @@
@@ -5,12 +5,12 @@ #define REST_MAXIMUM_EVENTS 2E9 #include <TFile.h> -#include <TFileMerger.h> #include <TKey.h> #include "TRestAnalysisTree.h" #include "TRestEvent.h" #include "TRestMetadata.h" +#include "TRestTools.h" class TRestEventProcess;
Hunk 2@@ -44,6 +44,8 @@ class TRestRun : public TRestMetadata {
@@ -44,6 +44,8 @@ class TRestRun : public TRestMetadata {
// temp data members
std::vector<TString> fInputFileNames; //!
+ TRestRootFileHandle fInputFileOwner; //!
+ TRestRootFileHandle fOutputFileOwner; //!
TFile* fInputFile; //!
TFile* fOutputFile; //!
TRestEvent* fInputEvent; //!
Hunk 3@@ -98,6 +100,9 @@ class TRestRun : public TRestMetadata {
@@ -98,6 +100,9 @@ class TRestRun : public TRestMetadata {
TFile* UpdateOutputFile();
void PassOutputFile() {
+ if (!fOutputFileOwner.Close())
+ RESTError << "Failed to close the previously owned output file: " << fOutputFileOwner.Error()
+ << RESTendl;
fOutputFile = fInputFile;
fOutputFileName = fOutputFile->GetName();
}
Hunk 4@@ -259,6 +264,8 @@ class TRestRun : public TRestMetadata {
@@ -259,6 +264,8 @@ class TRestRun : public TRestMetadata {
// Constructor & Destructor
TRestRun();
explicit TRestRun(const std::string& filename);
+ TRestRun(const TRestRun&) = delete;
+ TRestRun& operator=(const TRestRun&) = delete;
~TRestRun();
ClassDefOverride(TRestRun, 6);
15source/framework/core/inc/TRestThread.hFull proposal vs master baseownershiprisk: highimpact: high+7 / −1
- Intent
- Make worker-file ownership explicit and hand completed worker outputs to the centralized merge path.
- Invariant
- A file has one owner; worker writes must finish and close before merge or cleanup.
- Human review
- Check multi-worker teardown and failure propagation, not just the mechanical Open substitution.
diff --git a/source/framework/core/inc/TRestThread.h b/source/framework/core/inc/TRestThread.h index addfbbce..e719b33d 100644 --- a/source/framework/core/inc/TRestThread.h +++ b/source/framework/core/inc/TRestThread.h
Hunk 1@@ -3,7 +3,6 @@
@@ -3,7 +3,6 @@ #define RestCore_TRestThread #include <TFile.h> -#include <TFileMerger.h> #include <TKey.h> #include <TObject.h> #include <TString.h>
Hunk 2@@ -19,6 +18,7 @@
@@ -19,6 +18,7 @@
#include "TRestEventProcess.h"
#include "TRestMetadata.h"
#include "TRestProcessRunner.h"
+#include "TRestTools.h"
/// Threaded worker of a process chain
class TRestThread {
Hunk 3@@ -30,6 +30,7 @@ class TRestThread {
@@ -30,6 +30,7 @@ class TRestThread {
TRestAnalysisTree* fAnalysisTree; //!
TRestEvent* fInputEvent; //!
TRestEvent* fOutputEvent; //!
+ TRestRootFileHandle fOutputFileOwner; //!
TFile* fOutputFile; //!
TTree* fEventTree; //!
Hunk 4@@ -63,6 +64,11 @@ class TRestThread {
@@ -63,6 +64,11 @@ class TRestThread {
inline Int_t GetThreadId() const { return fThreadId; }
inline TRestEvent* GetInputEvent() { return fInputEvent; }
inline TFile* GetOutputFile() { return fOutputFile; };
+ bool CloseOutputFile() {
+ const bool result = fOutputFileOwner.Close();
+ fOutputFile = nullptr;
+ return result;
+ }
inline TRestEvent* GetOutputEvent() { return fProcessNullReturned ? 0 : fOutputEvent; }
inline Int_t GetProcessnum() const { return fProcessChain.size(); }
inline TRestEventProcess* GetProcess(int i) const { return fProcessChain[i]; }
16source/framework/core/src/TRestAnalysisTree.cxxFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+3 / −2
- Intent
- Use an explicit destination for metadata/tree writing instead of relying on ambient ROOT directory state.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/source/framework/core/src/TRestAnalysisTree.cxx b/source/framework/core/src/TRestAnalysisTree.cxx index d52950b2..e5849daa 100644 --- a/source/framework/core/src/TRestAnalysisTree.cxx +++ b/source/framework/core/src/TRestAnalysisTree.cxx
Hunk 1@@ -92,6 +92,7 @@
@@ -92,6 +92,7 @@ #include "TRestStringHelper.h" #include "TRestStringOutput.h" +#include "TRestTools.h" using namespace std;
Hunk 2@@ -1217,8 +1218,8 @@ Int_t TRestAnalysisTree::WriteAsTTree(const char* name, Int_t option, Int_t bufs
@@ -1217,8 +1218,8 @@ Int_t TRestAnalysisTree::WriteAsTTree(const char* name, Int_t option, Int_t bufs
/// <param name="file"> The input file that contains another AnalysisTree with same run id </param>
/// <returns></returns>
Bool_t TRestAnalysisTree::AddChainFile(const string& _file) {
- auto file = std::unique_ptr<TFile>{TFile::Open(_file.c_str(), "update")};
- if (!file->IsOpen()) {
+ auto file = TRestRootFileHandle::Open(_file, TRestRootFileMode::Read);
+ if (!file) {
RESTWarning << "TRestAnalysisTree::AddChainFile(): failed to open file " << _file << RESTendl;
return false;
}
17source/framework/core/src/TRestDataSet.cxxFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+6 / −2
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/source/framework/core/src/TRestDataSet.cxx b/source/framework/core/src/TRestDataSet.cxx index 22ba8334..aeae7cfb 100644 --- a/source/framework/core/src/TRestDataSet.cxx +++ b/source/framework/core/src/TRestDataSet.cxx
Hunk 1@@ -991,11 +991,15 @@ void TRestDataSet::Export(const std::string& filename, std::vector<std::string>
@@ -991,11 +991,15 @@ void TRestDataSet::Export(const std::string& filename, std::vector<std::string>
} else if (TRestTools::GetFileNameExtension(filename) == "root") {
fDataFrame.Snapshot("AnalysisTree", filename);
- TFile* f = TFile::Open(filename.c_str(), "UPDATE");
+ auto file = TRestRootFileHandle::Open(filename, TRestRootFileMode::Update);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
std::string name = this->GetName();
if (name.empty()) name = "mock";
this->Write(name.c_str());
- f->Close();
+ if (!file.Close()) RESTError << file.Error() << RESTendl;
} else {
RESTWarning << "TRestDataSet::Export. Extension " << TRestTools::GetFileNameExtension(filename)
<< " not recognized" << RESTendl;
18source/framework/core/src/TRestDataSetPlot.cxxFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+7 / −1
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/source/framework/core/src/TRestDataSetPlot.cxx b/source/framework/core/src/TRestDataSetPlot.cxx index 7e33a4ea..7d651737 100644 --- a/source/framework/core/src/TRestDataSetPlot.cxx +++ b/source/framework/core/src/TRestDataSetPlot.cxx
Hunk 1@@ -295,6 +295,7 @@
@@ -295,6 +295,7 @@ #include "TCanvas.h" #include "TDirectory.h" +#include "TRestTools.h" #include "TStyle.h" ClassImp(TRestDataSetPlot);
Hunk 2@@ -991,13 +992,18 @@ void TRestDataSetPlot::PlotCombinedCanvas() {
@@ -991,13 +992,18 @@ void TRestDataSetPlot::PlotCombinedCanvas() {
combinedCanvas.Print(fOutputFileName.c_str());
// In case of root file save also the histograms
if (TRestTools::GetFileNameExtension(fOutputFileName) == "root") {
- std::unique_ptr<TFile> f(TFile::Open(fOutputFileName.c_str(), "UPDATE"));
+ auto file = TRestRootFileHandle::Open(fOutputFileName, TRestRootFileMode::Update);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
for (auto& plots : fPlots) {
for (auto& hist : plots.histos) {
hist.histo->Write();
}
}
this->Write();
+ if (!file.Close()) RESTError << file.Error() << RESTendl;
}
}
19source/framework/core/src/TRestMetadataPlot.cxxFull proposal vs master basecall-site adoptionrisk: mediumimpact: medium+7 / −3
- Intent
- Replace direct writable ROOT opens with checked RAII handles and explicit close/error handling at this existing call site.
- Invariant
- Retain the caller's intended READ/RECREATE/UPDATE semantics and keep borrowed pointers within the owner's lifetime.
- Human review
- Check error exits and ROOT directory selection. Most changes here adopt the interface rather than add a new schema algorithm.
diff --git a/source/framework/core/src/TRestMetadataPlot.cxx b/source/framework/core/src/TRestMetadataPlot.cxx index 55b1bc46..ba9c2cd6 100644 --- a/source/framework/core/src/TRestMetadataPlot.cxx +++ b/source/framework/core/src/TRestMetadataPlot.cxx
Hunk 1@@ -935,10 +935,14 @@ void TRestMetadataPlot::GenerateCanvas() {
@@ -935,10 +935,14 @@ void TRestMetadataPlot::GenerateCanvas() {
// If the extension of the canvas save file is ROOT we store also the histograms
if (TRestTools::isRootFile((string)fCanvasSave)) {
- TFile* f = new TFile(fCanvasSave, "UPDATE");
- f->cd();
+ auto file = TRestRootFileHandle::Open(fCanvasSave.Data(), TRestRootFileMode::Update);
+ if (!file) {
+ RESTError << file.Error() << RESTendl;
+ return;
+ }
+ file->cd();
for (unsigned int n = 0; n < graphCollectionAll.size(); n++) graphCollectionAll[n]->Write();
- f->Close();
+ if (!file.Close()) RESTError << file.Error() << RESTendl;
}
// Save this class to the root file
20source/framework/core/src/TRestProcessRunner.cxxFull proposal vs master baseownershiprisk: highimpact: high+59 / −18
- Intent
- Make worker-file ownership explicit and hand completed worker outputs to the centralized merge path.
- Invariant
- A file has one owner; worker writes must finish and close before merge or cleanup.
- Human review
- Check multi-worker teardown and failure propagation, not just the mechanical Open substitution.
diff --git a/source/framework/core/src/TRestProcessRunner.cxx b/source/framework/core/src/TRestProcessRunner.cxx index 2ff4d4ff..c28b7194 100644 --- a/source/framework/core/src/TRestProcessRunner.cxx +++ b/source/framework/core/src/TRestProcessRunner.cxx
Hunk 1@@ -32,6 +32,7 @@
@@ -32,6 +32,7 @@ #include "TROOT.h" #include "TRestManager.h" #include "TRestThread.h" +#include "TRestTools.h" #ifdef WIN32 #include <io.h>
Hunk 2@@ -344,7 +345,17 @@ void TRestProcessRunner::RunProcess() {
@@ -344,7 +345,17 @@ void TRestProcessRunner::RunProcess() {
TString filename = fRunInfo->FormFormat(fRunInfo->GetOutputFileName());
fOutputDataFileName = filename;
- fOutputDataFile = new TFile(filename, "recreate");
+ auto output = TRestRootFileHandle::Open(filename.Data(), TRestRootFileMode::Recreate);
+ if (!output) {
+ RESTError << output.Error() << RESTendl;
+ exit(1);
+ }
+ if (fOutputDataFileOwner && !fOutputDataFileOwner.Close()) {
+ RESTError << fOutputDataFileOwner.Error() << RESTendl;
+ exit(1);
+ }
+ fOutputDataFileOwner = std::move(output);
+ fOutputDataFile = fOutputDataFileOwner.Get();
// set compression level here will cause problem in pipeline
// we must set in each threadCompression
// fOutputDataFile->SetCompressionLevel(fFile);
Hunk 3@@ -890,15 +901,30 @@ void TRestProcessRunner::FillThreadEventFunc(TRestThread* t) {
@@ -890,15 +901,30 @@ void TRestProcessRunner::FillThreadEventFunc(TRestThread* t) {
// write some information to the first(main) data file
fRunInfo->SetNFilesSplit(fNFilesSplit);
if (fOutputDataFile->GetName() != fOutputDataFileName) {
- auto Mainfile = std::unique_ptr<TFile>{TFile::Open(fOutputDataFileName, "update")};
- WriteProcessesMetadata();
- Mainfile->Write(0, TObject::kOverwrite);
- Mainfile->Close();
+ auto mainFile =
+ TRestRootFileHandle::Open(fOutputDataFileName.Data(), TRestRootFileMode::Update);
+ if (!mainFile) {
+ RESTError << mainFile.Error() << RESTendl;
+ exit(1);
+ }
+ WriteProcessesMetadata(mainFile.Get());
+ mainFile->Write(0, TObject::kOverwrite);
+ if (!mainFile.Close()) {
+ RESTError << mainFile.Error() << RESTendl;
+ exit(1);
+ }
} else {
WriteProcessesMetadata();
}
- TFile* newfile = new TFile(fOutputDataFileName + "." + ToString(fNFilesSplit), "recreate");
+ const std::string splitFileName =
+ std::string(fOutputDataFileName.Data()) + "." + ToString(fNFilesSplit);
+ auto splitFile = TRestRootFileHandle::Open(splitFileName, TRestRootFileMode::Recreate);
+ if (!splitFile) {
+ RESTError << splitFile.Error() << RESTendl;
+ exit(1);
+ }
+ TFile* newfile = splitFile.Get();
TBranch* branch = nullptr;
fAnalysisTree->SetDirectory(newfile);
Hunk 4@@ -922,9 +948,12 @@ void TRestProcessRunner::FillThreadEventFunc(TRestThread* t) {
@@ -922,9 +948,12 @@ void TRestProcessRunner::FillThreadEventFunc(TRestThread* t) {
}
fOutputDataFile->Write(nullptr, TObject::kOverwrite);
- fOutputDataFile->Close();
- delete fOutputDataFile;
- fOutputDataFile = newfile;
+ if (!fOutputDataFileOwner.Close()) {
+ RESTError << fOutputDataFileOwner.Error() << RESTendl;
+ exit(1);
+ }
+ fOutputDataFileOwner = std::move(splitFile);
+ fOutputDataFile = fOutputDataFileOwner.Get();
} else {
RESTError << "internal error!" << RESTendl;
}
Hunk 5@@ -964,8 +993,11 @@ void TRestProcessRunner::ConfigOutputFile() {
@@ -964,8 +993,11 @@ void TRestProcessRunner::ConfigOutputFile() {
// close file
fOutputDataFile->Write();
- fOutputDataFile->Close();
- delete fOutputDataFile;
+ if (!fOutputDataFileOwner.Close()) {
+ RESTError << fOutputDataFileOwner.Error() << RESTendl;
+ exit(1);
+ }
+ fOutputDataFile = nullptr;
// merge process's data file to the main file
// we must call this method before writing process metadata,
Hunk 6@@ -984,17 +1016,22 @@ void TRestProcessRunner::ConfigOutputFile() {
@@ -984,17 +1016,22 @@ void TRestProcessRunner::ConfigOutputFile() {
}
///////////////////////////////////////////////
-/// \brief Write process metadata to fOutputDataFile
+/// \brief Write process metadata to the requested file, or fOutputDataFile by default.
///
-void TRestProcessRunner::WriteProcessesMetadata() {
- fOutputDataFile->cd();
+void TRestProcessRunner::WriteProcessesMetadata(TFile* destination) {
+ TFile* output = destination != nullptr ? destination : fOutputDataFile;
+ if (output == nullptr || !output->IsOpen() || !output->IsWritable()) {
+ RESTError << "Cannot write process metadata without a writable destination" << RESTendl;
+ return;
+ }
+ output->cd();
this->Write(nullptr, TObject::kWriteDelete);
- if (fRunInfo->GetFileProcess() != nullptr) {
+ if (fRunInfo != nullptr && fRunInfo->GetFileProcess() != nullptr) {
fRunInfo->GetFileProcess()->Write(nullptr, kOverwrite);
}
- for (int i = 0; i < fProcessNumber; i++) {
+ for (int i = 0; i < fProcessNumber && !fThreads.empty(); i++) {
fThreads[0]->GetProcess(i)->Write(nullptr, kOverwrite);
}
}
Hunk 7@@ -1013,10 +1050,14 @@ void TRestProcessRunner::MergeOutputFile() {
@@ -1013,10 +1050,14 @@ void TRestProcessRunner::MergeOutputFile() {
for (int i = 0; i < fThreadNumber; i++) {
TFile* f = fThreads[i]->GetOutputFile();
if (f != nullptr) {
+ const std::string threadFileName = f->GetName();
f->Write(nullptr, TObject::kOverwrite);
- f->Close();
+ if (!fThreads[i]->CloseOutputFile()) {
+ RESTError << "Failed to close thread output file " << threadFileName << RESTendl;
+ exit(1);
+ }
+ files_to_merge.push_back(threadFileName);
}
- files_to_merge.push_back(f->GetName());
}
if (TRestTools::fileExists((string)fOutputDataFileName)) {
21source/framework/core/src/TRestRun.cxxFull proposal vs master baseownershiprisk: highimpact: high+95 / −40
- Intent
- Give TRestRun owned input/output handles while retaining raw aliases and its legacy string-mode entry point; route worker merging through the transaction helper.
- Invariant
- No shallow copies of live run ownership. CloseFile clears owners before the replacement paths; redundant later closes are removed.
- Human review
- Follow tree/file destruction order, split-file processing and post-merge metadata writes. Existing macros must not start owning borrowed pointers.
diff --git a/source/framework/core/src/TRestRun.cxx b/source/framework/core/src/TRestRun.cxx index b6596ac3..7fa48e58 100644 --- a/source/framework/core/src/TRestRun.cxx +++ b/source/framework/core/src/TRestRun.cxx
Hunk 1@@ -41,6 +41,7 @@
@@ -41,6 +41,7 @@ #include "TRestDataBase.h" #include "TRestEventProcess.h" #include "TRestManager.h" +#include "TRestTools.h" #include "TRestVersion.h" using namespace std;
Hunk 2@@ -330,12 +331,13 @@ void TRestRun::OpenInputFile(int i) {
@@ -330,12 +331,13 @@ void TRestRun::OpenInputFile(int i) {
///
void TRestRun::OpenInputFile(const TString& filename, const string& mode) {
CloseFile();
- if (!filename.Contains("http") && !TRestTools::fileExists((string)filename)) {
+ const bool isRemote = TRestTools::IsRemoteRootPath(filename.Data());
+ if (!isRemote && !TRestTools::fileExists((string)filename)) {
RESTError << "input file \"" << filename << "\" does not exist!" << RESTendl;
exit(1);
}
- if (!filename.Contains("http")) ReadFileInfo((string)filename);
+ if (!isRemote) ReadFileInfo((string)filename);
// add to fInputFileNames in case it is opening a new file
bool inList = false;
Hunk 3@@ -351,7 +353,25 @@ void TRestRun::OpenInputFile(const TString& filename, const string& mode) {
@@ -351,7 +353,25 @@ void TRestRun::OpenInputFile(const TString& filename, const string& mode) {
}
if (TRestTools::isRootFile((string)filename)) {
- fInputFile = TFile::Open(filename, mode.c_str());
+ TString normalizedMode(mode);
+ normalizedMode.ToUpper();
+ TRestRootFileMode fileMode;
+ if (normalizedMode.IsNull() || normalizedMode == "READ" || normalizedMode == "OPEN") {
+ fileMode = TRestRootFileMode::Read;
+ } else if (normalizedMode == "UPDATE") {
+ fileMode = TRestRootFileMode::Update;
+ } else {
+ RESTError << "TRestRun::OpenInputFile(): unsupported ROOT mode '" << mode
+ << "'. Supported modes are READ/OPEN and UPDATE." << RESTendl;
+ exit(1);
+ }
+ auto inputFile = TRestRootFileHandle::Open(filename.Data(), fileMode);
+ if (!inputFile) {
+ RESTError << inputFile.Error() << RESTendl;
+ exit(1);
+ }
+ fInputFileOwner = std::move(inputFile);
+ fInputFile = fInputFileOwner.Get();
if (GetMetadataClass("TRestRun", fInputFile)) {
// This should be the values in RML (if it was initialized using RML)
Hunk 4@@ -979,47 +999,57 @@ TString TRestRun::FormFormat(const TString& FilenameFormat) {
@@ -979,47 +999,57 @@ TString TRestRun::FormFormat(const TString& FilenameFormat) {
///
/// If output file name is not given(=""), then it will recreate the output file
/// according to fOutputFileName. Otherwise it will update the given file. File
-/// Merging is by calling TFileMerger. After this, it will format the merged file name.
-/// This method is used to create output file after TRestProcessRunner is finished.
-/// The metadata objects will also be written into the file.
+/// The merge is built and validated in a same-directory temporary file before
+/// replacing the formatted destination. This method is used to create the
+/// output file after TRestProcessRunner is finished. The metadata objects will
+/// also be written into the file.
TFile* TRestRun::MergeToOutputFile(vector<string> filenames, string outputfilename) {
RESTDebug << "TRestRun::FormOutputFile. target : " << outputfilename << RESTendl;
- string filename;
- TFileMerger* m = new TFileMerger(false);
- if (outputfilename.empty()) {
- filename = fOutputFileName;
- RESTInfo << "Creating file : " << filename << RESTendl;
- m->OutputFile(filename.c_str(), "RECREATE");
- } else {
- filename = outputfilename;
- RESTInfo << "Updating file : " << filename << RESTendl;
- m->OutputFile(filename.c_str(), "UPDATE");
- }
-
- RESTDebug << "TRestRun::FormOutputFile. Starting to add files" << RESTendl;
+ const string filename = outputfilename.empty() ? string(fOutputFileName.Data()) : outputfilename;
+ fOutputFileName = FormFormat(filename);
+ const string finalFilename = fOutputFileName.Data();
+ RESTInfo << (outputfilename.empty() ? "Creating file : " : "Updating file : ") << finalFilename
+ << RESTendl;
- for (unsigned int i = 0; i < filenames.size(); i++) {
- m->AddFile(filenames[i].c_str(), false);
+ std::error_code targetStatusError;
+ const bool targetExists = !outputfilename.empty() && std::filesystem::exists(filename, targetStatusError);
+ if (targetStatusError) {
+ fOutputFileName = "";
+ RESTError << "Cannot inspect merge target '" << filename << "': " << targetStatusError.message()
+ << RESTendl;
+ exit(1);
}
-
- if (m->Merge()) {
- for (unsigned int i = 0; i < filenames.size(); i++) {
- remove(filenames[i].c_str());
- }
- } else {
+ const string existingTarget = targetExists ? filename : "";
+ std::string mergeError;
+ if (!TRestTools::MergeRootFilesTransactionally(finalFilename, filenames, existingTarget, true,
+ &mergeError)) {
fOutputFileName = "";
- RESTError << "(Merge files) failed to merge process files." << RESTendl;
+ RESTError << "(Merge files) " << mergeError << RESTendl;
exit(1);
}
- delete m;
-
- // we rename the created output file
- fOutputFileName = FormFormat(filename);
- rename(filename.c_str(), fOutputFileName);
+ if (!outputfilename.empty() && filename != finalFilename && !TRestTools::IsRemoteRootPath(filename)) {
+ std::error_code removalError;
+ std::filesystem::remove(filename, removalError);
+ if (removalError)
+ RESTWarning << "Merged output is valid, but the pre-format target '" << filename
+ << "' could not be removed: " << removalError.message() << RESTendl;
+ }
// write metadata into the output file
- fOutputFile = new TFile(fOutputFileName, "update");
+ auto outputFile = TRestRootFileHandle::Open(finalFilename, TRestRootFileMode::Update);
+ if (!outputFile) {
+ RESTError << outputFile.Error() << RESTendl;
+ fOutputFileName = "";
+ exit(1);
+ }
+ if (fOutputFileOwner && !fOutputFileOwner.Close()) {
+ RESTError << fOutputFileOwner.Error() << RESTendl;
+ fOutputFileName = "";
+ exit(1);
+ }
+ fOutputFileOwner = std::move(outputFile);
+ fOutputFile = fOutputFileOwner.Get();
RESTDebug << "TRestRun::FormOutputFile. Calling WriteWithDataBase()" << RESTendl;
this->WriteWithDataBase();
Hunk 5@@ -1040,7 +1070,13 @@ TFile* TRestRun::FormOutputFile() {
@@ -1040,7 +1070,13 @@ TFile* TRestRun::FormOutputFile() {
// remove unwanted "./" etc. from the path while resolving them
fOutputFileName = std::filesystem::weakly_canonical(fOutputFileName.Data());
- fOutputFile = new TFile(fOutputFileName, "recreate");
+ auto outputFile = TRestRootFileHandle::Open(fOutputFileName.Data(), TRestRootFileMode::Recreate);
+ if (!outputFile) {
+ RESTError << outputFile.Error() << RESTendl;
+ return nullptr;
+ }
+ fOutputFileOwner = std::move(outputFile);
+ fOutputFile = fOutputFileOwner.Get();
fAnalysisTree = new TRestAnalysisTree("AnalysisTree", "AnalysisTree");
fEventTree = new TTree("EventTree", "EventTree");
Hunk 6@@ -1057,8 +1093,16 @@ TFile* TRestRun::FormOutputFile() {
@@ -1057,8 +1093,16 @@ TFile* TRestRun::FormOutputFile() {
TFile* TRestRun::UpdateOutputFile() {
if (fOutputFile != nullptr) {
- if (fOutputFile->IsOpen()) {
- fOutputFile->ReOpen("update");
+ if (!fOutputFile->IsOpen()) {
+ RESTError << "TRestRun::UpdateOutputFile(): output file is closed" << RESTendl;
+ return nullptr;
+ }
+ if (!fOutputFile->IsWritable()) {
+ std::string updateError;
+ if (!TRestRootFileHandle::PrepareBorrowedUpdate(*fOutputFile, &updateError)) {
+ RESTError << updateError << RESTendl;
+ return nullptr;
+ }
}
fOutputFile->cd();
Hunk 7@@ -1188,14 +1232,24 @@ void TRestRun::CloseFile() {
@@ -1188,14 +1232,24 @@ void TRestRun::CloseFile() {
fInputEvent = nullptr;
if (fOutputFile != nullptr) {
fOutputFile->Write(0, TObject::kOverwrite);
- fOutputFile->Close();
- delete fOutputFile;
+ if (fOutputFileOwner.Get() == fOutputFile) {
+ if (!fOutputFileOwner.Close()) RESTError << fOutputFileOwner.Error() << RESTendl;
+ } else if (fOutputFile != fInputFile) {
+ fOutputFile->Close();
+ delete fOutputFile;
+ }
fOutputFile = nullptr;
}
if (fInputFile != nullptr) {
- fInputFile->Close();
+ if (fInputFileOwner.Get() == fInputFile) {
+ if (!fInputFileOwner.Close()) RESTError << fInputFileOwner.Error() << RESTendl;
+ } else {
+ fInputFile->Close();
+ }
fInputFile = nullptr;
}
+ if (fOutputFileOwner && !fOutputFileOwner.Close()) RESTError << fOutputFileOwner.Error() << RESTendl;
+ if (fInputFileOwner && !fInputFileOwner.Close()) RESTError << fInputFileOwner.Error() << RESTendl;
}
///////////////////////////////////////////////
Hunk 8@@ -1217,6 +1271,7 @@ void TRestRun::SetExtProcess(TRestEventProcess* p) {
@@ -1217,6 +1271,7 @@ void TRestRun::SetExtProcess(TRestEventProcess* p) {
fInputEvent->SetSubRunOrigin(fParentRunNumber);
fInputEvent->SetTimeStamp(fStartTime);
}
+ if (fInputFileOwner && !fInputFileOwner.Close()) RESTError << fInputFileOwner.Error() << RESTendl;
fInputFile = nullptr;
// we make sure external processes can access to analysis tree
fAnalysisTree = new TRestAnalysisTree("externalProcessAna", "externalProcessAna");
22source/framework/core/src/TRestThread.cxxFull proposal vs master baseownershiprisk: highimpact: high+22 / −2
- Intent
- Make worker-file ownership explicit and hand completed worker outputs to the centralized merge path.
- Invariant
- A file has one owner; worker writes must finish and close before merge or cleanup.
- Human review
- Check multi-worker teardown and failure propagation, not just the mechanical Open substitution.
diff --git a/source/framework/core/src/TRestThread.cxx b/source/framework/core/src/TRestThread.cxx index 1b56c018..7b691e7c 100644 --- a/source/framework/core/src/TRestThread.cxx +++ b/source/framework/core/src/TRestThread.cxx
Hunk 1@@ -245,7 +245,17 @@ void TRestThread::PrepareToProcess(bool* outputConfig) {
@@ -245,7 +245,17 @@ void TRestThread::PrepareToProcess(bool* outputConfig) {
if (fProcessChain.size() > 0) {
RESTDebug << "TRestThread: Creating file : " << threadFileName << RESTendl;
- fOutputFile = new TFile(threadFileName.c_str(), "recreate");
+ auto outputFile = TRestRootFileHandle::Open(threadFileName, TRestRootFileMode::Recreate);
+ if (!outputFile) {
+ RESTError << outputFile.Error() << RESTendl;
+ exit(1);
+ }
+ if (fOutputFileOwner && !fOutputFileOwner.Close()) {
+ RESTError << fOutputFileOwner.Error() << RESTendl;
+ exit(1);
+ }
+ fOutputFileOwner = std::move(outputFile);
+ fOutputFile = fOutputFileOwner.Get();
fOutputFile->SetCompressionLevel(fCompressionLevel);
fAnalysisTree = new TRestAnalysisTree("AnalysisTree_" + ToString(fThreadId), "dummyTree");
fAnalysisTree->DisableQuickObservableValueSetting();
Hunk 2@@ -393,7 +403,17 @@ void TRestThread::PrepareToProcess(bool* outputConfig) {
@@ -393,7 +403,17 @@ void TRestThread::PrepareToProcess(bool* outputConfig) {
string tmp = fHostRunner->GetInputEvent()->ClassName();
fInputEvent = REST_Reflection::Assembly(tmp);
fOutputEvent = fInputEvent;
- fOutputFile = new TFile(threadFileName.c_str(), "recreate");
+ auto outputFile = TRestRootFileHandle::Open(threadFileName, TRestRootFileMode::Recreate);
+ if (!outputFile) {
+ RESTError << outputFile.Error() << RESTendl;
+ exit(1);
+ }
+ if (fOutputFileOwner && !fOutputFileOwner.Close()) {
+ RESTError << fOutputFileOwner.Error() << RESTendl;
+ exit(1);
+ }
+ fOutputFileOwner = std::move(outputFile);
+ fOutputFile = fOutputFileOwner.Get();
fOutputFile->SetCompressionLevel(fCompressionLevel);
fOutputFile->cd();
23source/framework/test/io/CMakeLists.txtFull proposal vs master basetestsrisk: mediumimpact: medium+121 / −0
- Intent
- Build isolated old/current dictionary libraries and fixture executables as dependencies of the existing framework test target.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/CMakeLists.txt b/source/framework/test/io/CMakeLists.txt new file mode 100644 index 00000000..25d27080 --- /dev/null +++ b/source/framework/test/io/CMakeLists.txt
Hunk 1@@ -0,0 +1,121 @@
@@ -0,0 +1,121 @@
+set(FIXTURE_OUTPUT_DIR "${CMAKE_CURRENT_BINARY_DIR}/artifacts")
+
+function (add_rest_io_fixture version)
+ set(target "RestIOFixtureV${version}")
+ set(source_dir "${CMAKE_CURRENT_SOURCE_DIR}/v${version}")
+ set(dictionary "G__${target}")
+ root_generate_dictionary(
+ ${dictionary} "${source_dir}/TRestIOFixturePayload.h" LINKDEF
+ "${source_dir}/LinkDef.h")
+ add_library(
+ ${target} SHARED "${source_dir}/TRestIOFixturePayload.cxx"
+ "${CMAKE_CURRENT_BINARY_DIR}/${dictionary}.cxx")
+ target_include_directories(${target} PUBLIC "${source_dir}")
+ target_link_libraries(${target} PUBLIC ${ROOT_LIBRARIES})
+ set_target_properties(
+ ${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/v${version}")
+ add_custom_command(
+ TARGET ${target}
+ POST_BUILD
+ COMMAND
+ ${CMAKE_COMMAND} -E copy_if_different
+ "${CMAKE_CURRENT_BINARY_DIR}/lib${target}_rdict.pcm"
+ "$<TARGET_FILE_DIR:${target}>"
+ COMMAND
+ ${CMAKE_COMMAND} -E copy_if_different
+ "${CMAKE_CURRENT_BINARY_DIR}/lib${target}.rootmap"
+ "$<TARGET_FILE_DIR:${target}>")
+
+ add_executable("RestIOWriterV${version}" WriteFixture.cxx)
+ target_link_libraries("RestIOWriterV${version}" PRIVATE ${target}
+ ${ROOT_LIBRARIES})
+ set_target_properties(
+ "RestIOWriterV${version}" PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+endfunction ()
+
+add_rest_io_fixture(1)
+add_rest_io_fixture(2)
+
+add_executable(RestIOUpdater UpdateFixture.cxx)
+target_link_libraries(RestIOUpdater PRIVATE RestFramework ${ROOT_LIBRARIES})
+set_target_properties(RestIOUpdater PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+
+add_executable(RestIOLoadedUpdater UpdateFixtureLoaded.cxx)
+target_link_libraries(RestIOLoadedUpdater PRIVATE RestIOFixtureV2 RestFramework
+ ${ROOT_LIBRARIES})
+set_target_properties(
+ RestIOLoadedUpdater PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+
+add_executable(RestIOVerifier VerifyFixture.cxx)
+target_link_libraries(RestIOVerifier PRIVATE RestIOFixtureV2 ${ROOT_LIBRARIES})
+set_target_properties(RestIOVerifier PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+
+add_executable(RestIOVerifierV1 VerifyFixtureV1.cxx)
+target_link_libraries(RestIOVerifierV1 PRIVATE RestIOFixtureV1
+ ${ROOT_LIBRARIES})
+set_target_properties(RestIOVerifierV1 PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+
+add_executable(RestIOSchemaWriter WriteSchemaFixture.cxx)
+target_link_libraries(RestIOSchemaWriter PRIVATE RestIOFixtureV2
+ ${ROOT_LIBRARIES})
+set_target_properties(RestIOSchemaWriter PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+
+add_executable(RestIOAnalysisTreeVerifier VerifyAnalysisTree.cxx)
+target_link_libraries(RestIOAnalysisTreeVerifier PRIVATE RestFramework
+ ${ROOT_LIBRARIES})
+set_target_properties(
+ RestIOAnalysisTreeVerifier PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+
+add_dependencies(
+ testRestFramework
+ RestIOWriterV1
+ RestIOWriterV2
+ RestIOUpdater
+ RestIOLoadedUpdater
+ RestIOVerifier
+ RestIOVerifierV1
+ RestIOSchemaWriter
+ RestIOAnalysisTreeVerifier)
+target_compile_definitions(
+ testRestFramework
+ PRIVATE
+ REST_IO_WRITER_V1="$<TARGET_FILE:RestIOWriterV1>"
+ REST_IO_WRITER_V2="$<TARGET_FILE:RestIOWriterV2>"
+ REST_IO_UPDATER="$<TARGET_FILE:RestIOUpdater>"
+ REST_IO_LOADED_UPDATER="$<TARGET_FILE:RestIOLoadedUpdater>"
+ REST_IO_VERIFIER="$<TARGET_FILE:RestIOVerifier>"
+ REST_IO_VERIFIER_V1="$<TARGET_FILE:RestIOVerifierV1>"
+ REST_IO_SCHEMA_WRITER="$<TARGET_FILE:RestIOSchemaWriter>"
+ REST_IO_ANALYSIS_TREE_VERIFIER="$<TARGET_FILE:RestIOAnalysisTreeVerifier>"
+ REST_PAIR_BASE_SOURCE_FILE="${CMAKE_SOURCE_DIR}/pipeline/fileopening/v2.2.30_hits.root"
+)
+
+foreach (layout Old New)
+ set(target "RestIOConflict${layout}")
+ set(dictionary "G__${target}")
+ set(options)
+ if (layout STREQUAL "Old")
+ list(APPEND options -DREST_IO_OLD_LAYOUT)
+ endif ()
+ root_generate_dictionary(
+ ${dictionary} "${CMAKE_CURRENT_SOURCE_DIR}/ConflictPayload.h" LINKDEF
+ "${CMAKE_CURRENT_SOURCE_DIR}/ConflictLinkDef.h" OPTIONS ${options})
+ add_executable(${target} ConflictFixture.cxx
+ "${CMAKE_CURRENT_BINARY_DIR}/${dictionary}.cxx")
+ target_compile_options(${target} PRIVATE ${options})
+ target_link_libraries(${target} PRIVATE RestFramework ${ROOT_LIBRARIES})
+ set_target_properties(${target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+ add_dependencies(testRestFramework ${target})
+ target_compile_definitions(
+ testRestFramework
+ PRIVATE "REST_IO_CONFLICT_${layout}=\"$<TARGET_FILE:${target}>\"")
+endforeach ()
24source/framework/test/io/ConflictFixture.cxxFull proposal vs master basetestsrisk: mediumimpact: high+33 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/io/ConflictFixture.cxx b/source/framework/test/io/ConflictFixture.cxx new file mode 100644 index 00000000..2fed99da --- /dev/null +++ b/source/framework/test/io/ConflictFixture.cxx
Hunk 1@@ -0,0 +1,33 @@
@@ -0,0 +1,33 @@
+#include <TFile.h>
+#include <TStreamerInfo.h>
+
+#include <iostream>
+#include <string>
+
+#include "ConflictPayload.h"
+#include "TRestTools.h"
+
+ClassImp(TRestIOConflictPayload);
+
+int main(int argc, char** argv) {
+ if (argc != 2 && argc != 3) return 2;
+ // Establish the active dictionary before ROOT reads the conflicting file.
+ TRestIOConflictPayload::Class()->GetStreamerInfo()->Build();
+#ifdef REST_IO_OLD_LAYOUT
+ TFile file(argv[1], "CREATE");
+ if (file.IsZombie()) return 3;
+ TRestIOConflictPayload payload;
+ payload.Write("payload");
+ return 0;
+#else
+ if (argc == 3) {
+ std::string error;
+ const bool merged = TRestTools::MergeRootFilesTransactionally(argv[2], {argv[1]}, "", true, &error);
+ std::cout << "merged=" << merged << " error=" << error << '\n';
+ return merged ? 4 : 0;
+ }
+ auto file = TRestRootFileHandle::Open(argv[1], TRestRootFileMode::Update);
+ std::cout << "update=" << bool(file) << " error=" << file.Error() << '\n';
+ return file ? 4 : 0;
+#endif
+}
25source/framework/test/io/ConflictLinkDef.hFull proposal vs master basetestsrisk: mediumimpact: high+1 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/io/ConflictLinkDef.h b/source/framework/test/io/ConflictLinkDef.h new file mode 100644 index 00000000..7d8b688b --- /dev/null +++ b/source/framework/test/io/ConflictLinkDef.h
Hunk 1@@ -0,0 +1 @@
@@ -0,0 +1 @@ +#pragma link C++ class TRestIOConflictPayload + ;
26source/framework/test/io/ConflictPayload.hFull proposal vs master basetestsrisk: mediumimpact: high+12 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/io/ConflictPayload.h b/source/framework/test/io/ConflictPayload.h new file mode 100644 index 00000000..47445375 --- /dev/null +++ b/source/framework/test/io/ConflictPayload.h
Hunk 1@@ -0,0 +1,12 @@
@@ -0,0 +1,12 @@
+#include <TObject.h>
+
+// Deliberately invalid evolution: the layout changed without a version bump.
+class TRestIOConflictPayload : public TObject {
+ public:
+#ifdef REST_IO_OLD_LAYOUT
+ int fValue = 123;
+#else
+ double fValue = 123.5;
+#endif
+ ClassDef(TRestIOConflictPayload, 2);
+};
27source/framework/test/io/UpdateFixture.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+37 / −0
- Intent
- Perform checked updates without loading the fixture's user dictionary.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/UpdateFixture.cxx b/source/framework/test/io/UpdateFixture.cxx new file mode 100644 index 00000000..c719b9ff --- /dev/null +++ b/source/framework/test/io/UpdateFixture.cxx
Hunk 1@@ -0,0 +1,37 @@
@@ -0,0 +1,37 @@
+#include <TFile.h>
+#include <TNamed.h>
+
+#include <cstdlib>
+#include <iostream>
+#include <memory>
+#include <string>
+
+#include "TRestTools.h"
+
+int main(int argc, char** argv) {
+ if (argc != 3) return 2;
+ const std::string action = argv[2];
+ if (action == "preserve" || action == "preserve-with-auto-registration-disabled") {
+ if (action == "preserve-with-auto-registration-disabled") TFile::SetReadStreamerInfo(kFALSE);
+ auto file = TRestRootFileHandle::Open(argv[1], TRestRootFileMode::Update);
+ TFile::SetReadStreamerInfo(kTRUE);
+ if (!file) {
+ std::cerr << file.Error() << '\n';
+ return 3;
+ }
+ TNamed marker("unloaded-update", "dictionary intentionally unavailable");
+ marker.Write();
+ return file.Close() ? EXIT_SUCCESS : 4;
+ }
+
+ if (action == "borrowed-failure") {
+ std::unique_ptr<TFile> file(TFile::Open(argv[1], "READ"));
+ if (!file || file->IsZombie() || file->IsWritable()) return 5;
+ TRestTools::ForceNextRootUpdatePreflightFailureForTesting();
+ std::string error;
+ if (TRestRootFileHandle::PrepareBorrowedUpdate(*file, &error)) return 7;
+ if (file->IsWritable()) return 8;
+ return error.find("Forced writable ROOT preflight failure") != std::string::npos ? EXIT_SUCCESS : 9;
+ }
+ return 10;
+}
28source/framework/test/io/UpdateFixtureLoaded.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+20 / −0
- Intent
- Repeat updates with the modern dictionary already loaded, exercising ROOT's schema cache.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/UpdateFixtureLoaded.cxx b/source/framework/test/io/UpdateFixtureLoaded.cxx new file mode 100644 index 00000000..4c0043c9 --- /dev/null +++ b/source/framework/test/io/UpdateFixtureLoaded.cxx
Hunk 1@@ -0,0 +1,20 @@
@@ -0,0 +1,20 @@
+#include <TNamed.h>
+
+#include <cstdlib>
+
+#include "TRestIOFixturePayload.h"
+#include "TRestTools.h"
+
+int main(int argc, char** argv) {
+ if (argc != 2) return 2;
+ TRestIOFixturePayload::Class();
+ for (int update = 0; update < 3; ++update) {
+ auto file = TRestRootFileHandle::Open(argv[1], TRestRootFileMode::Update);
+ if (!file) return 3;
+ const std::string name = update == 0 ? "unloaded-update" : "loaded-update-" + std::to_string(update);
+ TNamed marker(name.c_str(), "loaded dictionary");
+ marker.Write();
+ if (!file.Close()) return 4;
+ }
+ return EXIT_SUCCESS;
+}
29source/framework/test/io/VerifyAnalysisTree.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+31 / −0
- Intent
- Read actual REST AnalysisTree values in a separate process after worker merging.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/VerifyAnalysisTree.cxx b/source/framework/test/io/VerifyAnalysisTree.cxx new file mode 100644 index 00000000..0af88e78 --- /dev/null +++ b/source/framework/test/io/VerifyAnalysisTree.cxx
Hunk 1@@ -0,0 +1,31 @@
@@ -0,0 +1,31 @@
+#include <TRestAnalysisTree.h>
+#include <TRestRun.h>
+
+#include <cmath>
+#include <cstdlib>
+#include <string>
+
+namespace {
+bool Near(double left, double right) { return std::abs(left - right) < 1e-12; }
+} // namespace
+
+int main(int argc, char** argv) {
+ if (argc != 5) return 2;
+ const double expectedTheta = std::stod(argv[2]);
+ const double expectedPhi = std::stod(argv[3]);
+ const double expectedEnergy = std::stod(argv[4]);
+
+ TRestRun run(argv[1]);
+ TRestAnalysisTree* tree = run.GetAnalysisTree();
+ if (tree == nullptr || tree->GetEntries() <= 100) return 3;
+
+ if (tree->Draw("theta", "", "goff", 1, 100) != 1 || !Near(tree->GetV1()[0], expectedTheta)) return 4;
+ if (tree->Draw("phi", "", "goff", 1, 100) != 1 || !Near(tree->GetV1()[0], expectedPhi)) return 5;
+ if (tree->Draw("totalEdep", "", "goff", 1, 100) != 1 || !Near(tree->GetV1()[0], expectedEnergy)) return 6;
+
+ run.GetEntry(100);
+ if (!Near(tree->GetObservableValue<double>("theta"), expectedTheta)) return 7;
+ if (!Near(tree->GetObservableValue<double>("phi"), expectedPhi)) return 8;
+ if (!Near(tree->GetObservableValue<double>("totalEdep"), expectedEnergy)) return 9;
+ return EXIT_SUCCESS;
+}
30source/framework/test/io/VerifyFixture.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+43 / −0
- Intent
- Use the modern dictionary to verify automatic collection evolution and renamed-field rules.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/VerifyFixture.cxx b/source/framework/test/io/VerifyFixture.cxx new file mode 100644 index 00000000..2a0d5d79 --- /dev/null +++ b/source/framework/test/io/VerifyFixture.cxx
Hunk 1@@ -0,0 +1,43 @@
@@ -0,0 +1,43 @@
+#include <TFile.h>
+#include <TNamed.h>
+
+#include <cmath>
+#include <cstdlib>
+#include <iostream>
+#include <memory>
+#include <string>
+
+#include "TRestIOFixturePayload.h"
+
+namespace {
+bool Near(double left, double right) { return std::abs(left - right) < 1e-12; }
+} // namespace
+
+int main(int argc, char** argv) {
+ if (argc != 2 && argc != 3) return 2;
+ std::unique_ptr<TFile> file(TFile::Open(argv[1], "READ"));
+ if (!file || file->IsZombie()) return 3;
+
+ std::unique_ptr<TRestIOFixturePayload> oldPayload(file->Get<TRestIOFixturePayload>("payload-v1"));
+ std::unique_ptr<TRestIOFixturePayload> newPayload(file->Get<TRestIOFixturePayload>("payload-v2"));
+ if (!oldPayload || !newPayload) return 4;
+ if (oldPayload->fValue != 11 || oldPayload->fSamples.size() != 2 ||
+ !Near(oldPayload->fSamples[0], 1.25) || !Near(oldPayload->fSamples[1], 2.5) ||
+ !Near(oldPayload->fRenamedCode, 37.0))
+ std::cerr << "Unexpected v1 payload after evolution: value=" << oldPayload->fValue
+ << " sample-count=" << oldPayload->fSamples.size()
+ << (oldPayload->fSamples.empty() ? "" : " first=" + std::to_string(oldPayload->fSamples[0]))
+ << " renamed-code=" << oldPayload->fRenamedCode << '\n';
+ if (oldPayload->fValue != 11 || oldPayload->fSamples.size() != 2 ||
+ !Near(oldPayload->fSamples[0], 1.25) || !Near(oldPayload->fSamples[1], 2.5) ||
+ !Near(oldPayload->fRenamedCode, 37.0))
+ return 5;
+ if (newPayload->fValue != 22 || newPayload->fSamples.size() != 2 ||
+ !Near(newPayload->fSamples[0], 3.75) || !Near(newPayload->fSamples[1], 5.0) ||
+ !Near(newPayload->fRenamedCode, 73.5) || !Near(newPayload->fExtra, 9.5))
+ return 6;
+ if (argc == 3 && std::string(argv[2]) == "require-update" &&
+ file->Get<TNamed>("unloaded-update") == nullptr)
+ return 7;
+ return EXIT_SUCCESS;
+}
31source/framework/test/io/VerifyFixtureV1.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+24 / −0
- Intent
- Reopen with the historical dictionary to verify old data still has usable schema metadata.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/VerifyFixtureV1.cxx b/source/framework/test/io/VerifyFixtureV1.cxx new file mode 100644 index 00000000..8454f22e --- /dev/null +++ b/source/framework/test/io/VerifyFixtureV1.cxx
Hunk 1@@ -0,0 +1,24 @@
@@ -0,0 +1,24 @@
+#include <TFile.h>
+
+#include <cmath>
+#include <cstdlib>
+#include <memory>
+
+#include "TRestIOFixturePayload.h"
+
+namespace {
+bool Near(float left, float right) { return std::abs(left - right) < 1e-6F; }
+} // namespace
+
+int main(int argc, char** argv) {
+ if (argc != 2) return 2;
+ std::unique_ptr<TFile> file(TFile::Open(argv[1], "READ"));
+ if (!file || file->IsZombie()) return 3;
+
+ std::unique_ptr<TRestIOFixturePayload> payload(file->Get<TRestIOFixturePayload>("payload-v1"));
+ if (!payload) return 4;
+ if (payload->fValue != 11 || payload->fSamples.size() != 2 || !Near(payload->fSamples[0], 1.25F) ||
+ !Near(payload->fSamples[1], 2.5F) || payload->fLegacyCode != 37)
+ return 5;
+ return EXIT_SUCCESS;
+}
32source/framework/test/io/WriteFixture.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+27 / −0
- Intent
- Create old/new payload files in isolated dictionary processes.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/WriteFixture.cxx b/source/framework/test/io/WriteFixture.cxx new file mode 100644 index 00000000..fc3e18d9 --- /dev/null +++ b/source/framework/test/io/WriteFixture.cxx
Hunk 1@@ -0,0 +1,27 @@
@@ -0,0 +1,27 @@
+#include <TFile.h>
+#include <TTree.h>
+
+#include <cstdlib>
+#include <memory>
+
+#include "TRestIOFixturePayload.h"
+
+int main(int argc, char** argv) {
+ if (argc != 3) return 2;
+ std::unique_ptr<TFile> file(TFile::Open(argv[1], argv[2]));
+ if (!file || file->IsZombie() || !file->IsWritable()) return 3;
+
+ TRestIOFixturePayload payload;
+ const char* key = TRestIOFixturePayload::Class_Version() == 1 ? "payload-v1" : "payload-v2";
+ file->WriteObject(&payload, key);
+ if (TRestIOFixturePayload::Class_Version() == 1) {
+ TTree events("events", "realistic tree-bearing update fixture");
+ double value = 42.5;
+ events.Branch("value", &value);
+ events.Fill();
+ events.Write();
+ }
+ file->Write();
+ file->Close();
+ return file->TestBit(TFile::kWriteError) ? 4 : EXIT_SUCCESS;
+}
33source/framework/test/io/WriteSchemaFixture.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+50 / −0
- Intent
- Construct controlled historical schema/rule records for preservation tests.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/WriteSchemaFixture.cxx b/source/framework/test/io/WriteSchemaFixture.cxx new file mode 100644 index 00000000..696fd11f --- /dev/null +++ b/source/framework/test/io/WriteSchemaFixture.cxx
Hunk 1@@ -0,0 +1,50 @@
@@ -0,0 +1,50 @@
+#include <TArrayC.h>
+#include <TFile.h>
+#include <TList.h>
+#include <TNamed.h>
+#include <TStreamerInfo.h>
+
+#include <cstdlib>
+#include <memory>
+#include <set>
+
+#include "TRestIOFixturePayload.h"
+
+int main(int argc, char** argv) {
+ if (argc != 3) return 2;
+ TRestIOFixturePayload::Class();
+
+ std::set<Int_t> numbers;
+ {
+ std::unique_ptr<TFile> source(TFile::Open(argv[1], "READ"));
+ if (!source || source->IsZombie()) return 3;
+ std::unique_ptr<TList> infos(source->GetStreamerInfoList());
+ if (!infos) return 4;
+ infos->SetOwner(kFALSE);
+ TIter next(infos.get());
+ while (TObject* object = next()) {
+ auto* info = dynamic_cast<TStreamerInfo*>(object);
+ if (info == nullptr) {
+ object->SetBit(TObject::kCanDelete);
+ continue;
+ }
+ info->BuildCheck(source.get());
+ if (info->GetNumber() > 0) numbers.insert(info->GetNumber());
+ }
+ infos->Clear();
+ }
+
+ TFile output(argv[2], "RECREATE");
+ if (!output.IsOpen() || output.IsZombie()) return 5;
+ TNamed marker("schema-only-worker", "schema-only-worker");
+ marker.Write();
+ TArrayC* classIndex = output.GetClassIndex();
+ if (classIndex == nullptr || numbers.empty()) return 6;
+ const Int_t maximumNumber = *numbers.rbegin();
+ if (maximumNumber >= classIndex->GetSize()) classIndex->Set(maximumNumber + 1);
+ for (const Int_t number : numbers) classIndex->fArray[number] = 1;
+ classIndex->fArray[0] = 1;
+ output.WriteStreamerInfo();
+ output.Close();
+ return output.TestBit(TFile::kWriteError) ? 7 : EXIT_SUCCESS;
+}
34source/framework/test/io/v1/LinkDef.hFull proposal vs master basetestsrisk: mediumimpact: medium+4 / −0
- Intent
- Define isolated versioned payload dictionaries, including automatic collection evolution and an explicit renamed-field rule.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/v1/LinkDef.h b/source/framework/test/io/v1/LinkDef.h new file mode 100644 index 00000000..2452c626 --- /dev/null +++ b/source/framework/test/io/v1/LinkDef.h
Hunk 1@@ -0,0 +1,4 @@
@@ -0,0 +1,4 @@ +#ifdef __CLING__ +#pragma link C++ class TRestIOFixturePayload + ; +#pragma link C++ class std::vector < float> + ; +#endif
35source/framework/test/io/v1/TRestIOFixturePayload.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+3 / −0
- Intent
- Define isolated versioned payload dictionaries, including automatic collection evolution and an explicit renamed-field rule.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/v1/TRestIOFixturePayload.cxx b/source/framework/test/io/v1/TRestIOFixturePayload.cxx new file mode 100644 index 00000000..f009d1ec --- /dev/null +++ b/source/framework/test/io/v1/TRestIOFixturePayload.cxx
Hunk 1@@ -0,0 +1,3 @@
@@ -0,0 +1,3 @@ +#include "TRestIOFixturePayload.h" + +ClassImp(TRestIOFixturePayload);
36source/framework/test/io/v1/TRestIOFixturePayload.hFull proposal vs master basetestsrisk: mediumimpact: medium+17 / −0
- Intent
- Define isolated versioned payload dictionaries, including automatic collection evolution and an explicit renamed-field rule.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/v1/TRestIOFixturePayload.h b/source/framework/test/io/v1/TRestIOFixturePayload.h new file mode 100644 index 00000000..251aff1f --- /dev/null +++ b/source/framework/test/io/v1/TRestIOFixturePayload.h
Hunk 1@@ -0,0 +1,17 @@
@@ -0,0 +1,17 @@
+#ifndef RestIOFixtureV1_TRestIOFixturePayload
+#define RestIOFixtureV1_TRestIOFixturePayload
+
+#include <TObject.h>
+
+#include <vector>
+
+class TRestIOFixturePayload : public TObject {
+ public:
+ int fValue = 11;
+ std::vector<float> fSamples{1.25F, 2.5F};
+ int fLegacyCode = 37;
+
+ ClassDef(TRestIOFixturePayload, 1);
+};
+
+#endif
37source/framework/test/io/v2/LinkDef.hFull proposal vs master basetestsrisk: mediumimpact: medium+6 / −0
- Intent
- Define isolated versioned payload dictionaries, including automatic collection evolution and an explicit renamed-field rule.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/v2/LinkDef.h b/source/framework/test/io/v2/LinkDef.h new file mode 100644 index 00000000..4b633374 --- /dev/null +++ b/source/framework/test/io/v2/LinkDef.h
Hunk 1@@ -0,0 +1,6 @@
@@ -0,0 +1,6 @@
+#ifdef __CLING__
+#pragma link C++ class TRestIOFixturePayload + ;
+#pragma link C++ class std::vector < double> + ;
+#pragma read sourceClass = "TRestIOFixturePayload" version = "[1]" source = "int fLegacyCode" targetClass = \
+ "TRestIOFixturePayload" target = "fRenamedCode" code = "{ fRenamedCode = onfile.fLegacyCode; }"
+#endif
38source/framework/test/io/v2/TRestIOFixturePayload.cxxFull proposal vs master basetestsrisk: mediumimpact: medium+3 / −0
- Intent
- Define isolated versioned payload dictionaries, including automatic collection evolution and an explicit renamed-field rule.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/v2/TRestIOFixturePayload.cxx b/source/framework/test/io/v2/TRestIOFixturePayload.cxx new file mode 100644 index 00000000..f009d1ec --- /dev/null +++ b/source/framework/test/io/v2/TRestIOFixturePayload.cxx
Hunk 1@@ -0,0 +1,3 @@
@@ -0,0 +1,3 @@ +#include "TRestIOFixturePayload.h" + +ClassImp(TRestIOFixturePayload);
39source/framework/test/io/v2/TRestIOFixturePayload.hFull proposal vs master basetestsrisk: mediumimpact: medium+18 / −0
- Intent
- Define isolated versioned payload dictionaries, including automatic collection evolution and an explicit renamed-field rule.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/v2/TRestIOFixturePayload.h b/source/framework/test/io/v2/TRestIOFixturePayload.h new file mode 100644 index 00000000..e98e5869 --- /dev/null +++ b/source/framework/test/io/v2/TRestIOFixturePayload.h
Hunk 1@@ -0,0 +1,18 @@
@@ -0,0 +1,18 @@
+#ifndef RestIOFixtureV2_TRestIOFixturePayload
+#define RestIOFixtureV2_TRestIOFixturePayload
+
+#include <TObject.h>
+
+#include <vector>
+
+class TRestIOFixturePayload : public TObject {
+ public:
+ int fValue = 22;
+ std::vector<double> fSamples{3.75, 5.0};
+ double fRenamedCode = 73.5;
+ double fExtra = 9.5;
+
+ ClassDef(TRestIOFixturePayload, 2);
+};
+
+#endif
40source/framework/test/src/RootIO.cxxFull proposal vs master basetestsrisk: mediumimpact: high+763 / −0
- Intent
- Exercise loaded and unloaded dictionaries, automatic float-to-double evolution, renamed-member rules, historical schemas, real AnalysisTree values, rollback and path policies. Additional regressions cover missing UPDATE destinations (including GainMap::Export), output aliases, new-output rollback, environment expansion and symlinks.
- Invariant
- Failed preflight does not change source bytes; failed publication retains sources and restores/removes output as appropriate.
- Human review
- Inspect the values being asserted, not merely successful opens. Fault injection covers controlled failure, not power-loss durability.
diff --git a/source/framework/test/src/RootIO.cxx b/source/framework/test/src/RootIO.cxx new file mode 100644 index 00000000..10187516 --- /dev/null +++ b/source/framework/test/src/RootIO.cxx
Hunk 1@@ -0,0 +1,763 @@
@@ -0,0 +1,763 @@
+#include <TArrayC.h>
+#include <TClassEdit.h>
+#include <TFile.h>
+#include <TGraph.h>
+#include <TH1D.h>
+#include <TKey.h>
+#include <TList.h>
+#include <TNamed.h>
+#include <TObjString.h>
+#include <TRestAnalysisTree.h>
+#include <TRestDataSetGainMap.h>
+#include <TRestProcessRunner.h>
+#include <TRestRun.h>
+#include <TRestTools.h>
+#include <TStreamerInfo.h>
+#include <TSystem.h>
+#include <TTree.h>
+#include <gtest/gtest.h>
+
+#include <algorithm>
+#include <cstdlib>
+#include <filesystem>
+#include <fstream>
+#include <iterator>
+#include <ostream>
+#include <set>
+#include <string>
+#include <tuple>
+#include <type_traits>
+#include <vector>
+
+namespace {
+namespace fs = std::filesystem;
+
+static_assert(!std::is_copy_constructible_v<TRestRun>);
+static_assert(!std::is_copy_assignable_v<TRestRun>);
+static_assert(std::is_move_constructible_v<TRestRootFileHandle>);
+static_assert(!std::is_copy_constructible_v<TRestRootFileHandle>);
+
+struct StreamerIdentity {
+ std::string name;
+ int version;
+ unsigned checksum;
+
+ bool operator<(const StreamerIdentity& other) const {
+ return std::tie(name, version, checksum) < std::tie(other.name, other.version, other.checksum);
+ }
+ bool operator==(const StreamerIdentity& other) const {
+ return name == other.name && version == other.version && checksum == other.checksum;
+ }
+};
+
+std::ostream& operator<<(std::ostream& output, const StreamerIdentity& identity) {
+ return output << identity.name << ':' << identity.version << ':' << identity.checksum;
+}
+
+struct SchemaInventory {
+ std::set<StreamerIdentity> infos;
+ std::set<std::string> rules;
+};
+
+class TemporaryDirectory {
+ private:
+ fs::path fPath;
+
+ public:
+ TemporaryDirectory() {
+ fPath = fs::temp_directory_path() /
+ ("rest-root-io-" + std::to_string(std::rand()) + "-" + std::to_string(std::rand()));
+ fs::create_directories(fPath);
+ }
+ ~TemporaryDirectory() {
+ std::error_code ignored;
+ fs::remove_all(fPath, ignored);
+ }
+ const fs::path& Path() const { return fPath; }
+};
+
+std::string ShellQuote(const std::string& value) {
+ std::string quoted = "'";
+ for (char character : value) {
+ if (character == '\'')
+ quoted += "'\\''";
+ else
+ quoted += character;
+ }
+ return quoted + "'";
+}
+
+int RunProcess(const std::string& executable, const std::vector<std::string>& arguments) {
+ std::string command = ShellQuote(executable);
+ for (const auto& argument : arguments) command += " " + ShellQuote(argument);
+ return std::system(command.c_str());
+}
+
+SchemaInventory ReadInventory(const fs::path& filename) {
+ SchemaInventory result;
+ std::unique_ptr<TFile> file(TFile::Open(filename.c_str(), "READ"));
+ EXPECT_TRUE(file != nullptr && file->IsOpen() && !file->IsZombie());
+ if (!file || file->IsZombie()) return result;
+
+ std::unique_ptr<TList> list(file->GetStreamerInfoList());
+ EXPECT_NE(list, nullptr);
+ if (!list) return result;
+ TIter next(list.get());
+ while (TObject* object = next()) {
+ if (auto* info = dynamic_cast<TStreamerInfo*>(object)) {
+ result.infos.insert({info->GetName(), info->GetClassVersion(), info->GetCheckSum()});
+ continue;
+ }
+ auto* rules = dynamic_cast<TList*>(object);
+ if (rules == nullptr || std::string(rules->GetName()) != "listOfRules") continue;
+ TIter nextRule(rules);
+ while (TObject* ruleObject = nextRule()) {
+ if (auto* rule = dynamic_cast<TObjString*>(ruleObject))
+ result.rules.insert(rule->GetString().Data());
+ }
+ }
+ return result;
+}
+
+std::set<StreamerIdentity> PayloadInfos(const SchemaInventory& inventory) {
+ std::set<StreamerIdentity> result;
+ std::copy_if(inventory.infos.begin(), inventory.infos.end(), std::inserter(result, result.end()),
+ [](const auto& identity) { return identity.name == "TRestIOFixturePayload"; });
+ return result;
+}
+
+std::vector<char> ReadBytes(const fs::path& filename) {
+ std::ifstream input(filename, std::ios::binary);
+ return {std::istreambuf_iterator<char>(input), std::istreambuf_iterator<char>()};
+}
+
+bool HasKeyClass(const fs::path& filename, const std::string& className) {
+ std::unique_ptr<TFile> file(TFile::Open(filename.c_str(), "READ"));
+ if (!file || file->IsZombie()) return false;
+ TIter next(file->GetListOfKeys());
+ while (TObject* object = next()) {
+ auto* key = dynamic_cast<TKey*>(object);
+ if (key != nullptr && className == key->GetClassName()) return true;
+ }
+ return false;
+}
+
+void CreateTwoVersionFixture(const fs::path& filename) {
+ ASSERT_EQ(RunProcess(REST_IO_WRITER_V1, {filename.string(), "RECREATE"}), 0);
+ ASSERT_EQ(RunProcess(REST_IO_WRITER_V2, {filename.string(), "UPDATE"}), 0);
+}
+
+void CreateNamedInput(const fs::path& filename, const char* name) {
+ auto file = TRestRootFileHandle::Open(filename.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed marker(name, name);
+ marker.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+}
+
+void CreatePairBaseFixture(const fs::path& filename) {
+ std::set<StreamerIdentity> found;
+ std::vector<Int_t> numbers;
+ bool allResolved = true;
+ {
+ std::unique_ptr<TFile> source(TFile::Open(REST_PAIR_BASE_SOURCE_FILE, "READ"));
+ ASSERT_TRUE(source != nullptr && source->IsOpen() && !source->IsZombie());
+ std::unique_ptr<TList> infos(source->GetStreamerInfoList());
+ ASSERT_NE(infos, nullptr);
+ infos->SetOwner(kFALSE);
+ TIter next(infos.get());
+ while (TObject* object = next()) {
+ auto* info = dynamic_cast<TStreamerInfo*>(object);
+ if (info == nullptr) {
+ object->SetBit(TObject::kCanDelete);
+ continue;
+ }
+ info->BuildCheck(source.get());
+ if (!TClassEdit::IsStdPairBase(info->GetName())) continue;
+ found.insert({info->GetName(), info->GetClassVersion(), info->GetCheckSum()});
+ if (info->GetNumber() > 0)
+ numbers.push_back(info->GetNumber());
+ else
+ allResolved = false;
+ }
+ infos->Clear();
+ }
+
+ const std::set<StreamerIdentity> expected = {{"__pair_base<int,double>", 1, 30697198},
+ {"__pair_base<int,int>", 1, 637428422}};
+ ASSERT_TRUE(allResolved);
+ ASSERT_EQ(found, expected);
+
+ auto file = TRestRootFileHandle::Open(filename.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed marker("pair-base-marker", "pair-base-marker");
+ marker.Write();
+ TArrayC* classIndex = file->GetClassIndex();
+ ASSERT_NE(classIndex, nullptr);
+ for (const Int_t number : numbers) {
+ if (number >= classIndex->GetSize()) classIndex->Set(number + 1);
+ classIndex->fArray[number] = 1;
+ }
+ classIndex->fArray[0] = 1;
+ file->WriteStreamerInfo();
+ ASSERT_TRUE(file.Close()) << file.Error();
+}
+
+void CreateAnalysisTreeTarget(const fs::path& filename) {
+ auto file = TRestRootFileHandle::Open(filename.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TRestAnalysisTree tree("AnalysisTree", "observable binding regression fixture");
+ tree.DisableQuickObservableValueSetting();
+ for (int entry = 0; entry < 120; ++entry) {
+ tree.SetObservableValue("theta", 2.0 + entry / 400.0);
+ tree.SetObservableValue("phi", 1.0 + entry / 400.0);
+ tree.SetObservableValue("totalEdep", 1000.0 + entry);
+ tree.Fill();
+ }
+ tree.Write();
+ TNamed marker("target-only", "must not be rewritten as a merge input");
+ marker.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+}
+
+void CreateWorkerTree(const fs::path& filename, int worker) {
+ auto file = TRestRootFileHandle::Open(filename.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TTree tree("worker", "worker merge payload");
+ tree.Branch("worker", &worker);
+ tree.Fill();
+ tree.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+}
+
+void CheckExistingAnalysisTreeMerge(int workerCount) {
+ TemporaryDirectory temporary;
+ const fs::path target = temporary.Path() / "analysis-target.root";
+ CreateAnalysisTreeTarget(target);
+ Long64_t originalSeek = 0;
+ Int_t originalKeyBytes = 0;
+ Int_t originalObjectBytes = 0;
+ {
+ std::unique_ptr<TFile> original(TFile::Open(target.c_str(), "READ"));
+ ASSERT_NE(original, nullptr);
+ TKey* key = original->GetKey("AnalysisTree");
+ ASSERT_NE(key, nullptr);
+ originalSeek = key->GetSeekKey();
+ originalKeyBytes = key->GetNbytes();
+ originalObjectBytes = key->GetObjlen();
+ }
+ std::vector<std::string> inputs;
+ for (int worker = 0; worker < workerCount; ++worker) {
+ const fs::path input = temporary.Path() / ("worker-" + std::to_string(worker) + ".root");
+ CreateWorkerTree(input, worker);
+ inputs.push_back(input.string());
+ }
+
+ std::string error;
+ ASSERT_TRUE(
+ TRestTools::MergeRootFilesTransactionally(target.string(), inputs, target.string(), false, &error))
+ << error;
+ EXPECT_EQ(RunProcess(REST_IO_ANALYSIS_TREE_VERIFIER, {target.string(), "2.25", "1.25", "1100"}), 0);
+
+ std::unique_ptr<TFile> merged(TFile::Open(target.c_str(), "READ"));
+ ASSERT_NE(merged, nullptr);
+ TKey* analysisKey = merged->GetKey("AnalysisTree");
+ ASSERT_NE(analysisKey, nullptr);
+ EXPECT_EQ(analysisKey->GetSeekKey(), originalSeek);
+ EXPECT_EQ(analysisKey->GetNbytes(), originalKeyBytes);
+ EXPECT_EQ(analysisKey->GetObjlen(), originalObjectBytes);
+ EXPECT_NE(merged->Get<TNamed>("target-only"), nullptr);
+ auto* workerTree = merged->Get<TTree>("worker");
+ ASSERT_NE(workerTree, nullptr);
+ EXPECT_EQ(workerTree->GetEntries(), workerCount);
+}
+} // namespace
+
+TEST(RootIO, UnloadedDictionaryPreservesAutomaticCollectionEvolutionAndRenamedFieldRule) {
+ TemporaryDirectory temporary;
+ const fs::path filename = temporary.Path() / "two-versions.root";
+ CreateTwoVersionFixture(filename);
+
+ // Establish the two independent ROOT behaviors before REST mutates the file:
+ // fSamples evolves automatically by retaining its name, while fLegacyCode
+ // reaches fRenamedCode exclusively through the embedded schema rule.
+ ASSERT_EQ(RunProcess(REST_IO_VERIFIER_V1, {filename.string()}), 0);
+ ASSERT_EQ(RunProcess(REST_IO_VERIFIER, {filename.string()}), 0);
+
+ const SchemaInventory before = ReadInventory(filename);
+ const auto payloadBefore = PayloadInfos(before);
+ ASSERT_EQ(payloadBefore.size(), 2);
+ ASSERT_TRUE(std::any_of(before.rules.begin(), before.rules.end(), [](const auto& rule) {
+ return rule.find("TRestIOFixturePayload") != std::string::npos &&
+ rule.find("fLegacyCode") != std::string::npos &&
+ rule.find("fRenamedCode") != std::string::npos;
+ }));
+ EXPECT_FALSE(std::any_of(before.rules.begin(), before.rules.end(), [](const auto& rule) {
+ return rule.find("TRestIOFixturePayload") != std::string::npos &&
+ rule.find("fSamples") != std::string::npos;
+ }));
+
+ ASSERT_EQ(RunProcess(REST_IO_UPDATER, {filename.string(), "preserve"}), 0);
+
+ const SchemaInventory after = ReadInventory(filename);
+ EXPECT_TRUE(
+ std::includes(after.infos.begin(), after.infos.end(), before.infos.begin(), before.infos.end()));
+ EXPECT_TRUE(
+ std::includes(after.rules.begin(), after.rules.end(), before.rules.begin(), before.rules.end()));
+ EXPECT_EQ(PayloadInfos(after), payloadBefore);
+ // Re-read with each matching dictionary to prove that both automatic
+ // collection evolution and the independent renamed-field rule still work.
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER_V1, {filename.string()}), 0);
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER, {filename.string(), "require-update"}), 0);
+}
+
+TEST(RootIO, UpdateResolvesTreeSchemaWhenAutomaticStreamerRegistrationIsDisabled) {
+ TemporaryDirectory temporary;
+ const fs::path filename = temporary.Path() / "disabled-auto-registration.root";
+ CreateTwoVersionFixture(filename);
+ const SchemaInventory before = ReadInventory(filename);
+ ASSERT_TRUE(std::any_of(before.infos.begin(), before.infos.end(),
+ [](const auto& identity) { return identity.name == "ROOT::TIOFeatures"; }));
+
+ ASSERT_EQ(RunProcess(REST_IO_UPDATER, {filename.string(), "preserve-with-auto-registration-disabled"}),
+ 0);
+
+ const SchemaInventory after = ReadInventory(filename);
+ EXPECT_TRUE(
+ std::includes(after.infos.begin(), after.infos.end(), before.infos.begin(), before.infos.end()));
+ EXPECT_TRUE(
+ std::includes(after.rules.begin(), after.rules.end(), before.rules.begin(), before.rules.end()));
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER_V1, {filename.string()}), 0);
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER, {filename.string(), "require-update"}), 0);
+}
+
+TEST(RootIO, BorrowedPreflightFailureLeavesFileByteIdentical) {
+ TemporaryDirectory temporary;
+ const fs::path filename = temporary.Path() / "preflight-failure.root";
+ CreateTwoVersionFixture(filename);
+ const auto before = ReadBytes(filename);
+
+ ASSERT_EQ(RunProcess(REST_IO_UPDATER, {filename.string(), "borrowed-failure"}), 0);
+
+ EXPECT_EQ(ReadBytes(filename), before);
+ EXPECT_EQ(PayloadInfos(ReadInventory(filename)).size(), 2);
+}
+
+TEST(RootIO, LoadedDictionaryCachePreservesAutomaticAndRuleBasedEvolutionAcrossUpdates) {
+ TemporaryDirectory temporary;
+ const fs::path filename = temporary.Path() / "loaded-updates.root";
+ CreateTwoVersionFixture(filename);
+ ASSERT_EQ(RunProcess(REST_IO_VERIFIER_V1, {filename.string()}), 0);
+ ASSERT_EQ(RunProcess(REST_IO_VERIFIER, {filename.string()}), 0);
+ const SchemaInventory before = ReadInventory(filename);
+ const auto payloadBefore = PayloadInfos(before);
+ ASSERT_EQ(payloadBefore.size(), 2);
+
+ ASSERT_EQ(RunProcess(REST_IO_LOADED_UPDATER, {filename.string()}), 0);
+
+ const SchemaInventory after = ReadInventory(filename);
+ EXPECT_TRUE(
+ std::includes(after.infos.begin(), after.infos.end(), before.infos.begin(), before.infos.end()));
+ EXPECT_TRUE(
+ std::includes(after.rules.begin(), after.rules.end(), before.rules.begin(), before.rules.end()));
+ EXPECT_EQ(PayloadInfos(after), payloadBefore);
+ std::unique_ptr<TFile> file(TFile::Open(filename.c_str(), "READ"));
+ ASSERT_NE(file, nullptr);
+ EXPECT_NE(file->Get<TNamed>("unloaded-update"), nullptr);
+ EXPECT_NE(file->Get<TNamed>("loaded-update-1"), nullptr);
+ EXPECT_NE(file->Get<TNamed>("loaded-update-2"), nullptr);
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER_V1, {filename.string()}), 0);
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER, {filename.string(), "require-update"}), 0);
+}
+
+TEST(RootIO, TransactionalMergeRetainsTargetOnlySchemaAndKeys) {
+ TemporaryDirectory temporary;
+ const fs::path target = temporary.Path() / "target.root";
+ const fs::path input = temporary.Path() / "input.root";
+
+ {
+ auto file = TRestRootFileHandle::Open(target.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TH1D targetOnly("target-only", "target-only", 10, 0, 10);
+ targetOnly.Fill(4);
+ targetOnly.Write();
+ TTree events("events", "events");
+ int value = 0;
+ events.Branch("value", &value);
+ events.Fill();
+ events.Fill();
+ events.Write();
+ auto* nested = file->mkdir("nested");
+ ASSERT_NE(nested, nullptr);
+ nested->cd();
+ TNamed targetNested("target-nested", "target nested key");
+ targetNested.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ const SchemaInventory targetSchema = ReadInventory(target);
+ {
+ auto file = TRestRootFileHandle::Open(input.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TGraph incoming;
+ incoming.SetName("incoming");
+ incoming.SetPoint(0, 1, 2);
+ incoming.Write();
+ TTree events("events", "events");
+ int value = 0;
+ events.Branch("value", &value);
+ events.Fill();
+ events.Fill();
+ events.Fill();
+ events.Write();
+ auto* nested = file->mkdir("incoming-nested");
+ ASSERT_NE(nested, nullptr);
+ nested->cd();
+ TNamed inputNested("input-nested", "input nested key");
+ inputNested.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(target.string(), {input.string()}, target.string(),
+ false, &error))
+ << error;
+
+ std::unique_ptr<TFile> merged(TFile::Open(target.c_str(), "READ"));
+ ASSERT_NE(merged, nullptr);
+ EXPECT_NE(merged->Get<TH1D>("target-only"), nullptr);
+ EXPECT_NE(merged->Get<TGraph>("incoming"), nullptr);
+ EXPECT_NE(merged->Get<TNamed>("nested/target-nested"), nullptr);
+ EXPECT_NE(merged->Get<TNamed>("incoming-nested/input-nested"), nullptr);
+ auto* events = merged->Get<TTree>("events");
+ ASSERT_NE(events, nullptr);
+ // Preserve TFileMerger UPDATE semantics: incoming same-name trees replace
+ // the target tree, while target-only objects remain untouched.
+ EXPECT_EQ(events->GetEntries(), 3);
+ const SchemaInventory finalSchema = ReadInventory(target);
+ EXPECT_TRUE(std::includes(finalSchema.infos.begin(), finalSchema.infos.end(), targetSchema.infos.begin(),
+ targetSchema.infos.end()));
+}
+
+TEST(RootIO, TransactionalMergeAcceptsRootManagedPairBaseNormalization) {
+ TemporaryDirectory temporary;
+ const fs::path pairBase = temporary.Path() / "pair-base.root";
+ const fs::path ordinary = temporary.Path() / "ordinary.root";
+ const fs::path output = temporary.Path() / "merged.root";
+ CreatePairBaseFixture(pairBase);
+ CreateNamedInput(ordinary, "ordinary-marker");
+
+ const SchemaInventory before = ReadInventory(pairBase);
+ ASSERT_EQ(std::count_if(before.infos.begin(), before.infos.end(),
+ [](const auto& identity) { return TClassEdit::IsStdPairBase(identity.name); }),
+ 2);
+
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(
+ output.string(), {pairBase.string(), ordinary.string()}, "", false, &error))
+ << error;
+ std::unique_ptr<TFile> merged(TFile::Open(output.c_str(), "READ"));
+ ASSERT_NE(merged, nullptr);
+ EXPECT_NE(merged->Get<TNamed>("pair-base-marker"), nullptr);
+ EXPECT_NE(merged->Get<TNamed>("ordinary-marker"), nullptr);
+}
+
+TEST(RootIO, TransactionalMergePreservesHistoricalUserSchemasAndRules) {
+ TemporaryDirectory temporary;
+ const fs::path legacy = temporary.Path() / "two-versions.root";
+ const fs::path ordinary = temporary.Path() / "ordinary.root";
+ CreateTwoVersionFixture(legacy);
+ CreateNamedInput(ordinary, "ordinary-marker");
+ const SchemaInventory before = ReadInventory(legacy);
+
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(legacy.string(), {ordinary.string()},
+ legacy.string(), false, &error))
+ << error;
+
+ const SchemaInventory after = ReadInventory(legacy);
+ const auto payloadBefore = PayloadInfos(before);
+ EXPECT_TRUE(
+ std::includes(after.infos.begin(), after.infos.end(), payloadBefore.begin(), payloadBefore.end()));
+ EXPECT_TRUE(
+ std::includes(after.rules.begin(), after.rules.end(), before.rules.begin(), before.rules.end()));
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER_V1, {legacy.string()}), 0);
+ EXPECT_EQ(RunProcess(REST_IO_VERIFIER, {legacy.string()}), 0);
+}
+
+TEST(RootIO, TransactionalMergePreservesWorkerOnlyHistoricalSchemasAndRules) {
+ TemporaryDirectory temporary;
+ const fs::path legacy = temporary.Path() / "two-versions.root";
+ const fs::path schemaOnly = temporary.Path() / "schema-only.root";
+ const fs::path target = temporary.Path() / "ordinary.root";
+ CreateTwoVersionFixture(legacy);
+ ASSERT_EQ(RunProcess(REST_IO_SCHEMA_WRITER, {legacy.string(), schemaOnly.string()}), 0);
+ CreateNamedInput(target, "ordinary-marker");
+ const SchemaInventory expected = ReadInventory(schemaOnly);
+ ASSERT_FALSE(expected.rules.empty());
+ ASSERT_FALSE(PayloadInfos(expected).empty());
+
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(target.string(), {schemaOnly.string()},
+ target.string(), false, &error))
+ << error;
+
+ const SchemaInventory after = ReadInventory(target);
+ EXPECT_TRUE(
+ std::includes(after.infos.begin(), after.infos.end(), expected.infos.begin(), expected.infos.end()));
+ EXPECT_TRUE(
+ std::includes(after.rules.begin(), after.rules.end(), expected.rules.begin(), expected.rules.end()));
+ std::unique_ptr<TFile> merged(TFile::Open(target.c_str(), "READ"));
+ ASSERT_NE(merged, nullptr);
+ EXPECT_NE(merged->Get<TNamed>("schema-only-worker"), nullptr);
+}
+
+TEST(RootIO, TransactionalUpdateKeepsRestAnalysisTreeValuesWithOneWorker) {
+ CheckExistingAnalysisTreeMerge(1);
+}
+
+TEST(RootIO, TransactionalUpdateKeepsRestAnalysisTreeValuesWithTwoWorkers) {
+ CheckExistingAnalysisTreeMerge(2);
+}
+
+TEST(RootIO, TransactionRejectsIncompatibleSameNameClasses) {
+ TemporaryDirectory temporary;
+ const fs::path target = temporary.Path() / "target.root";
+ const fs::path input = temporary.Path() / "input.root";
+ {
+ auto file = TRestRootFileHandle::Open(target.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed conflict("conflict", "target class");
+ conflict.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ {
+ auto file = TRestRootFileHandle::Open(input.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TH1D conflict("conflict", "input class", 10, 0, 10);
+ conflict.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ const auto targetBefore = ReadBytes(target);
+ const auto inputBefore = ReadBytes(input);
+
+ std::string error;
+ EXPECT_FALSE(TRestTools::MergeRootFilesTransactionally(target.string(), {input.string()}, target.string(),
+ true, &error));
+ EXPECT_NE(error.find("incompatible classes"), std::string::npos);
+ EXPECT_EQ(ReadBytes(target), targetBefore);
+ EXPECT_EQ(ReadBytes(input), inputBefore);
+}
+
+TEST(RootIO, TransactionFailurePreservesOriginalAndSources) {
+ TemporaryDirectory temporary;
+ const fs::path target = temporary.Path() / "target.root";
+ const fs::path input = temporary.Path() / "input.root";
+ const fs::path missing = temporary.Path() / "missing.root";
+ {
+ auto file = TRestRootFileHandle::Open(target.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed original("original", "must survive");
+ original.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ {
+ auto file = TRestRootFileHandle::Open(input.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed incoming("incoming", "must survive");
+ incoming.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ const auto targetBefore = ReadBytes(target);
+ const auto inputBefore = ReadBytes(input);
+
+ std::string error;
+ EXPECT_FALSE(TRestTools::MergeRootFilesTransactionally(
+ target.string(), {input.string(), missing.string()}, target.string(), true, &error));
+ EXPECT_FALSE(error.empty());
+ EXPECT_EQ(ReadBytes(target), targetBefore);
+ EXPECT_TRUE(fs::exists(input));
+ EXPECT_EQ(ReadBytes(input), inputBefore);
+ EXPECT_FALSE(fs::exists(missing));
+}
+
+TEST(RootIO, PostReplacementValidationFailureRollsBackAndKeepsSources) {
+ TemporaryDirectory temporary;
+ const fs::path target = temporary.Path() / "target.root";
+ const fs::path input = temporary.Path() / "input.root";
+ {
+ auto file = TRestRootFileHandle::Open(target.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed original("original", "must be restored byte-for-byte");
+ original.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ {
+ auto file = TRestRootFileHandle::Open(input.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed incoming("incoming", "must not be removed after rollback");
+ incoming.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ const auto targetBefore = ReadBytes(target);
+ const auto inputBefore = ReadBytes(input);
+
+ TRestTools::ForceNextTransactionalMergeValidationFailureForTesting();
+ std::string error;
+ EXPECT_FALSE(TRestTools::MergeRootFilesTransactionally(target.string(), {input.string()}, target.string(),
+ true, &error));
+ EXPECT_NE(error.find("Forced post-replacement"), std::string::npos);
+ EXPECT_EQ(ReadBytes(target), targetBefore);
+ EXPECT_TRUE(fs::exists(input));
+ EXPECT_EQ(ReadBytes(input), inputBefore);
+}
+
+TEST(RootIO, SplitMetadataUsesExplicitDestination) {
+ TemporaryDirectory temporary;
+ const fs::path mainName = temporary.Path() / "main.root";
+ const fs::path splitName = temporary.Path() / "split.root";
+ auto main = TRestRootFileHandle::Open(mainName.string(), TRestRootFileMode::Recreate);
+ auto split = TRestRootFileHandle::Open(splitName.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(main) << main.Error();
+ ASSERT_TRUE(split) << split.Error();
+
+ split->cd();
+ TRestProcessRunner runner;
+ runner.WriteProcessesMetadata(main.Get());
+ ASSERT_TRUE(main.Close()) << main.Error();
+ ASSERT_TRUE(split.Close()) << split.Error();
+
+ EXPECT_TRUE(HasKeyClass(mainName, "TRestProcessRunner"));
+ EXPECT_FALSE(HasKeyClass(splitName, "TRestProcessRunner"));
+}
+
+TEST(RootIO, RemoteDestinationsAreRejectedButReadPolicyIsPermissive) {
+ const std::string remote = "root://127.0.0.1:1/not-present.root";
+ EXPECT_TRUE(TRestTools::IsRemoteRootPath(remote));
+ auto update = TRestRootFileHandle::Open(remote, TRestRootFileMode::Update);
+ EXPECT_FALSE(update);
+ EXPECT_NE(update.Error().find("refuses to mutate remote"), std::string::npos);
+ auto recreate =
+ TRestRootFileHandle::Open("https://127.0.0.1/not-present.root", TRestRootFileMode::Recreate);
+ EXPECT_FALSE(recreate);
+ EXPECT_NE(recreate.Error().find("refuses to mutate remote"), std::string::npos);
+
+ auto read = TRestRootFileHandle::Open(remote, TRestRootFileMode::Read);
+ EXPECT_FALSE(read);
+ EXPECT_EQ(read.Error().find("refuses"), std::string::npos);
+}
+
+TEST(RootIO, FileUrlReadAndMoveLifecycle) {
+ TemporaryDirectory temporary;
+ const fs::path filename = fs::absolute(temporary.Path() / "lifecycle.root");
+ auto owner = TRestRootFileHandle::Open(filename.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(owner) << owner.Error();
+ auto moved = std::move(owner);
+ EXPECT_FALSE(owner);
+ TNamed marker("marker", "value");
+ marker.Write();
+ ASSERT_TRUE(moved.Close()) << moved.Error();
+
+ const std::string fileUrl = "file://" + filename.string();
+ EXPECT_FALSE(TRestTools::IsRemoteRootPath(fileUrl));
+ auto read = TRestRootFileHandle::Open(fileUrl, TRestRootFileMode::Read);
+ ASSERT_TRUE(read) << read.Error();
+ EXPECT_NE(read->Get<TNamed>("marker"), nullptr);
+}
+
+TEST(RootIO, TRestRunRejectsUnsupportedInputModes) {
+ TemporaryDirectory temporary;
+ const fs::path filename = temporary.Path() / "input.root";
+ {
+ auto file = TRestRootFileHandle::Open(filename.string(), TRestRootFileMode::Recreate);
+ ASSERT_TRUE(file) << file.Error();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ }
+ EXPECT_EXIT(
+ {
+ TRestRun run;
+ run.OpenInputFile(filename.c_str(), "RECREATE");
+ },
+ ::testing::ExitedWithCode(1), ".*");
+}
+
+TEST(RootIO, UpdateCreatesMissingFileLikeRootWithoutReplacingExistingData) {
+ TemporaryDirectory temporary;
+ const auto path = temporary.Path() / "new.root";
+ auto file = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Update);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed marker("original", "UPDATE creates a missing file");
+ marker.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ auto updated = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Update);
+ ASSERT_TRUE(updated) << updated.Error();
+ EXPECT_NE(updated->Get<TNamed>("original"), nullptr);
+ EXPECT_TRUE(updated.Close()) << updated.Error();
+
+ const auto gainPath = temporary.Path() / "gain.root";
+ TRestDataSetGainMap gainMap;
+ gainMap.Export(gainPath.string());
+ ASSERT_TRUE(fs::exists(gainPath));
+ EXPECT_TRUE(HasKeyClass(gainPath, "TRestDataSetGainMap"));
+}
+
+TEST(RootIO, MergeCleanupNeverDeletesOutputReferencedByPathAlias) {
+ TemporaryDirectory temporary;
+ const auto path = temporary.Path() / "target.root";
+ CreateNamedInput(path, "survivor");
+ const auto alias = temporary.Path() / "." / "target.root";
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(path.string(), {alias.string()}, "", true, &error))
+ << error;
+ auto file = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Read);
+ ASSERT_TRUE(file) << file.Error();
+ EXPECT_NE(file->Get<TNamed>("survivor"), nullptr);
+}
+
+TEST(RootIO, FailedPublicationOfNewOutputRemovesCandidateAndPreservesInput) {
+ TemporaryDirectory temporary;
+ const auto input = temporary.Path() / "input.root";
+ const auto output = temporary.Path() / "output.root";
+ CreateNamedInput(input, "survivor");
+ const auto before = ReadBytes(input);
+ TRestTools::ForceNextTransactionalMergeValidationFailureForTesting();
+ std::string error;
+ EXPECT_FALSE(
+ TRestTools::MergeRootFilesTransactionally(output.string(), {input.string()}, "", true, &error));
+ EXPECT_NE(error.find("Forced post-replacement"), std::string::npos);
+ EXPECT_FALSE(fs::exists(output));
+ EXPECT_EQ(ReadBytes(input), before);
+ EXPECT_EQ(std::distance(fs::directory_iterator(temporary.Path()), fs::directory_iterator()), 1);
+}
+
+TEST(RootIO, UpdateHonorsRootEnvironmentVariableExpansion) {
+ TemporaryDirectory temporary;
+ const auto path = temporary.Path() / "expanded.root";
+ CreateNamedInput(path, "original");
+ gSystem->Setenv("REST_IO_TEST_DIRECTORY", temporary.Path().c_str());
+ auto file = TRestRootFileHandle::Open("$REST_IO_TEST_DIRECTORY/expanded.root", TRestRootFileMode::Update);
+ gSystem->Unsetenv("REST_IO_TEST_DIRECTORY");
+ ASSERT_TRUE(file) << file.Error();
+ EXPECT_NE(file->Get<TNamed>("original"), nullptr);
+ EXPECT_TRUE(file.Close()) << file.Error();
+}
+
+TEST(RootIO, MergeUpdatesSymbolicLinkDestinationWithoutReplacingTheLink) {
+ TemporaryDirectory temporary;
+ const auto target = temporary.Path() / "target.root";
+ const auto alias = temporary.Path() / "alias.root";
+ const auto input = temporary.Path() / "input.root";
+ CreateNamedInput(target, "original");
+ CreateNamedInput(input, "incoming");
+ std::error_code ec;
+ fs::create_symlink(target, alias, ec);
+ if (ec) GTEST_SKIP() << "Symlinks unavailable: " << ec.message();
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(alias.string(), {input.string()}, alias.string(),
+ true, &error))
+ << error;
+ EXPECT_TRUE(fs::is_symlink(alias));
+ auto file = TRestRootFileHandle::Open(target.string(), TRestRootFileMode::Read);
+ ASSERT_TRUE(file) << file.Error();
+ EXPECT_NE(file->Get<TNamed>("original"), nullptr);
+ EXPECT_NE(file->Get<TNamed>("incoming"), nullptr);
+}
41source/framework/test/src/RootIOConflict.cxxFull proposal vs master basetestsrisk: mediumimpact: high+67 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/src/RootIOConflict.cxx b/source/framework/test/src/RootIOConflict.cxx new file mode 100644 index 00000000..e97de8ef --- /dev/null +++ b/source/framework/test/src/RootIOConflict.cxx
Hunk 1@@ -0,0 +1,67 @@
@@ -0,0 +1,67 @@
+#include <TFile.h>
+#include <TNamed.h>
+#include <TUUID.h>
+#include <gtest/gtest.h>
+
+#include <cstdlib>
+#include <filesystem>
+#include <fstream>
+#include <iterator>
+#include <string>
+#include <vector>
+
+namespace {
+std::vector<char> Bytes(const std::filesystem::path& path) {
+ std::ifstream input(path, std::ios::binary);
+ return {std::istreambuf_iterator<char>(input), std::istreambuf_iterator<char>()};
+}
+
+class RootIOConflict : public ::testing::Test {
+ protected:
+ std::filesystem::path directory;
+
+ void SetUp() override {
+ directory =
+ std::filesystem::temp_directory_path() / (std::string("rest-conflict-") + TUUID().AsString());
+ ASSERT_TRUE(std::filesystem::create_directory(directory));
+ }
+
+ void TearDown() override { std::filesystem::remove_all(directory); }
+
+ int Run(const char* executable, const std::vector<std::filesystem::path>& arguments) {
+ auto quote = [](const std::string& value) {
+ std::string result = "'";
+ for (const char c : value) result += c == '\'' ? "'\\''" : std::string(1, c);
+ return result + "'";
+ };
+ std::string command = quote(executable);
+ for (const auto& argument : arguments) command += " " + quote(argument.string());
+ return std::system(command.c_str());
+ }
+};
+} // namespace
+
+TEST_F(RootIOConflict, RejectUpdateWithoutChangingHistoricalSchemaOrBytes) {
+ const auto file = directory / "old.root";
+ ASSERT_EQ(Run(REST_IO_CONFLICT_Old, {file}), 0);
+ const auto before = Bytes(file);
+ ASSERT_FALSE(before.empty());
+ EXPECT_EQ(Run(REST_IO_CONFLICT_New, {file}), 0);
+ EXPECT_EQ(Bytes(file), before);
+}
+
+TEST_F(RootIOConflict, RejectWorkerSchemaConflictWithoutReplacingTargetOrRemovingInput) {
+ const auto source = directory / "old.root";
+ const auto target = directory / "target.root";
+ ASSERT_EQ(Run(REST_IO_CONFLICT_Old, {source}), 0);
+ {
+ TFile output(target.c_str(), "CREATE");
+ TNamed marker("marker", "original target");
+ marker.Write();
+ }
+ const auto sourceBefore = Bytes(source);
+ const auto targetBefore = Bytes(target);
+ EXPECT_EQ(Run(REST_IO_CONFLICT_New, {source, target}), 0);
+ EXPECT_EQ(Bytes(source), sourceBefore);
+ EXPECT_EQ(Bytes(target), targetBefore);
+}
42source/framework/tools/inc/TRestTools.hFull proposal vs master baseROOT corerisk: highimpact: critical+69 / −0
- Intent
- Expose the typed mode, move-only file owner, borrowed-update adapter and transactional merge API.
- Invariant
- Get() is a borrowed pointer. Writers explicitly check Close(); destruction alone cannot report failure.
- Human review
- Check ownership transfer, API lifetime contracts and public error semantics.
diff --git a/source/framework/tools/inc/TRestTools.h b/source/framework/tools/inc/TRestTools.h index 649df089..63813f2d 100644 --- a/source/framework/tools/inc/TRestTools.h +++ b/source/framework/tools/inc/TRestTools.h
Hunk 1@@ -57,6 +57,48 @@ EXTERN_DEF std::string REST_USER_PATH;
@@ -57,6 +57,48 @@ EXTERN_DEF std::string REST_USER_PATH;
EXTERN_DEF std::string REST_TMP_PATH;
EXTERN_DEF std::map<std::string, std::string> REST_ARGS;
+class TFile;
+class TRestTools;
+
+enum class TRestRootFileMode { Read, Recreate, Update };
+
+/// Move-only owner for ROOT files opened by REST.
+///
+/// UPDATE mode preserves all semantically required historical user schemas and
+/// embedded rules before the writable file is exposed. A failed open or
+/// preservation produces an invalid handle, queryable through operator bool()
+/// and Error().
+class TRestRootFileHandle {
+ private:
+ friend class TRestTools;
+
+ std::unique_ptr<TFile> fFile;
+ std::string fError;
+
+ public:
+ TRestRootFileHandle() = default;
+ static TRestRootFileHandle Open(const std::string& filename, TRestRootFileMode mode);
+
+ TRestRootFileHandle(const TRestRootFileHandle&) = delete;
+ TRestRootFileHandle& operator=(const TRestRootFileHandle&) = delete;
+ TRestRootFileHandle(TRestRootFileHandle&& other) noexcept;
+ TRestRootFileHandle& operator=(TRestRootFileHandle&& other) noexcept;
+ ~TRestRootFileHandle();
+
+ explicit operator bool() const noexcept { return fFile != nullptr; }
+ TFile* Get() const noexcept { return fFile.get(); }
+ TFile* operator->() const noexcept { return fFile.get(); }
+ TFile& operator*() const noexcept { return *fFile; }
+ const std::string& Error() const noexcept { return fError; }
+
+ bool Close() noexcept;
+
+ /// Validate an already-owned READ file, then transition it safely to UPDATE.
+ ///
+ /// The file remains read-only when validation fails before ReOpen.
+ static bool PrepareBorrowedUpdate(TFile& file, std::string* error = nullptr);
+};
+
/// A generic class with useful static methods.
class TRestTools {
public:
Hunk 2@@ -121,6 +163,33 @@ class TRestTools {
@@ -121,6 +163,33 @@ class TRestTools {
static bool isRunFile(const std::string& filename);
static bool isDataSet(const std::string& filename);
static bool isURL(const std::string& filename);
+ static bool IsRemoteRootPath(const std::string& filename);
+
+ /// Merge into a same-directory temporary ROOT file and replace the local
+ /// destination only after the merge and StreamerInfo validation succeed.
+ ///
+ /// `existingTarget`, when non-empty, seeds the temporary output and is
+ /// opened through the checked UPDATE path; only `inputFiles` are then
+ /// merged into it. Remote inputs are supported by ROOT, but `outputFile`
+ /// must resolve to a local file URL/path. Local `inputFiles` are removed
+ /// only after a successful replacement when `removeInputsOnSuccess` is
+ /// true.
+ static bool MergeRootFilesTransactionally(const std::string& outputFile,
+ const std::vector<std::string>& inputFiles,
+ const std::string& existingTarget = "",
+ bool removeInputsOnSuccess = true,
+ std::string* error = nullptr);
+
+#ifdef REST_TESTING_ENABLED
+ /// Force the next writable-open preflight to fail before the READ handle
+ /// transitions to UPDATE, so tests can verify the no-mutation guarantee.
+ static void ForceNextRootUpdatePreflightFailureForTesting();
+
+ /// Force the next transactional merge to fail its post-replacement
+ /// validation so tests can exercise rollback of an already-replaced file.
+ static void ForceNextTransactionalMergeValidationFailureForTesting();
+#endif
+
static bool isPathWritable(const std::string& path);
static bool isAbsolutePath(const std::string& path);
static std::string RemoveMultipleSlash(std::string);
43source/framework/tools/src/TRestTools.cxxFull proposal vs master baseROOT corerisk: highimpact: critical+731 / −0
- Intent
- Centralize schema snapshots, rule registration, checked writable opens and transactional merges. Inventory and resolution share one pass; exception unwinding handles internal cleanup.
- Invariant
- Preserve exact historical user schema identities and rules; reject incompatible resolution before writes; validate both candidate and installed merge results.
- Human review
- Audit BuildCheck ownership, the narrow STL normalization exception, filesystem identity checks, and every failure/rollback boundary. This is the main semantic review.
diff --git a/source/framework/tools/src/TRestTools.cxx b/source/framework/tools/src/TRestTools.cxx index 44ef9b01..96196f52 100644 --- a/source/framework/tools/src/TRestTools.cxx +++ b/source/framework/tools/src/TRestTools.cxx
Hunk 1@@ -43,10 +43,22 @@
@@ -43,10 +43,22 @@ /// #include "TRestTools.h" +#include <TArrayC.h> #include <TClass.h> +#include <TClassEdit.h> +#include <TDirectory.h> #include <TFile.h> +#include <TFileMerger.h> #include <TKey.h> +#include <TList.h> +#include <TObjString.h> +#include <TROOT.h> +#include <TSchemaRule.h> +#include <TSchemaRuleSet.h> +#include <TStreamerInfo.h> #include <TSystem.h> +#include <TTree.h> +#include <TUUID.h> #include <TUrl.h> #include <regex>
Hunk 2@@ -57,7 +69,10 @@
@@ -57,7 +69,10 @@ #ifdef WIN32 #include <io.h> +#include <windows.h> #else +#include <sys/stat.h> + #include "unistd.h" #endif // !WIN32
Hunk 3@@ -65,18 +80,567 @@
@@ -65,18 +80,567 @@
#include <array>
#endif
+#include <atomic>
+#include <cerrno>
#include <chrono>
+#include <cstring>
#include <filesystem>
#include <iostream>
#include <limits>
#include <memory>
+#include <optional>
+#include <stdexcept>
#include <thread>
+#include <tuple>
#include "TRestStringHelper.h"
#include "TRestStringOutput.h"
using namespace std;
+namespace {
+#ifdef REST_TESTING_ENABLED
+std::atomic_bool gForceNextRootUpdatePreflightFailure{false};
+std::atomic_bool gForceNextMergeValidationFailure{false};
+#endif
+
+struct StreamerInfoIdentity {
+ std::string name;
+ Int_t version;
+ UInt_t checksum;
+
+ bool operator<(const StreamerInfoIdentity& other) const {
+ return std::tie(name, version, checksum) < std::tie(other.name, other.version, other.checksum);
+ }
+};
+
+struct RootSchemaSnapshot {
+ std::set<StreamerInfoIdentity> streamerInfos;
+ std::set<std::string> schemaRules;
+ std::set<Int_t> numbers;
+};
+
+bool IsImplementationSchema(const std::string& name) {
+ return TClassEdit::IsStdClass(name.c_str()) || TClassEdit::IsStdPairBase(name.c_str());
+}
+
+struct RootContentEntry {
+ std::string className;
+ std::optional<Long64_t> treeEntries;
+};
+
+using RootContentManifest = std::map<std::string, RootContentEntry>;
+
+struct LocalFileIdentity {
+ std::uintmax_t size;
+ std::filesystem::file_time_type modificationTime;
+#ifndef WIN32
+ std::uintmax_t device;
+ std::uintmax_t inode;
+#endif
+
+ bool operator==(const LocalFileIdentity& other) const {
+ return size == other.size && modificationTime == other.modificationTime
+#ifndef WIN32
+ && device == other.device && inode == other.inode
+#endif
+ ;
+ }
+};
+
+std::optional<std::filesystem::path> LocalRootPath(const std::string& filename) {
+ TUrl url(filename.c_str(), kTRUE);
+ if (!url.IsValid() || std::string(url.GetProtocol()) != "file") return std::nullopt;
+ TString expanded(url.GetFile());
+ if (gSystem->ExpandPathName(expanded)) return std::nullopt;
+ return std::filesystem::path(expanded.Data());
+}
+
+std::optional<LocalFileIdentity> CaptureLocalFileIdentity(const std::filesystem::path& path,
+ std::string& error) {
+ std::error_code ec;
+ LocalFileIdentity identity;
+ identity.size = std::filesystem::file_size(path, ec);
+ if (ec) {
+ error = "Cannot inspect size of " + path.string() + ": " + ec.message();
+ return std::nullopt;
+ }
+ identity.modificationTime = std::filesystem::last_write_time(path, ec);
+ if (ec) {
+ error = "Cannot inspect modification time of " + path.string() + ": " + ec.message();
+ return std::nullopt;
+ }
+#ifndef WIN32
+ struct stat status {};
+ if (::stat(path.c_str(), &status) != 0) {
+ error = "Cannot inspect filesystem identity of " + path.string() + ": " + std::strerror(errno);
+ return std::nullopt;
+ }
+ identity.device = static_cast<std::uintmax_t>(status.st_dev);
+ identity.inode = static_cast<std::uintmax_t>(status.st_ino);
+#endif
+ return identity;
+}
+
+std::string NormalizeSchemaRule(const std::string& rule) {
+ ROOT::TSchemaRule parsed;
+ if (!parsed.SetFromRule(rule.c_str())) return "";
+ TString normalized;
+ parsed.AsString(normalized);
+ return normalized.Data();
+}
+
+bool ReadSchemaSnapshot(TFile& file, RootSchemaSnapshot& snapshot, std::string& error, bool resolve = false) {
+ if (!file.IsOpen() || file.IsZombie()) {
+ error = "Cannot inspect StreamerInfos in a closed or invalid ROOT file";
+ return false;
+ }
+ if (file.GetSeekInfo() == 0) return true;
+
+ std::unique_ptr<TList> infos(file.GetStreamerInfoList());
+ if (infos == nullptr) {
+ error = "ROOT could not read the existing StreamerInfo record";
+ return false;
+ }
+ infos->SetOwner(kTRUE);
+
+ TIter next(infos.get());
+ while (TObject* object = next()) {
+ if (auto* info = dynamic_cast<TStreamerInfo*>(object)) {
+ snapshot.streamerInfos.insert({info->GetName(), info->GetClassVersion(), info->GetCheckSum()});
+ continue;
+ }
+
+ auto* rules = dynamic_cast<TList*>(object);
+ if (rules == nullptr || std::string(rules->GetName()) != "listOfRules") continue;
+ TIter nextRule(rules);
+ while (TObject* ruleObject = nextRule()) {
+ auto* rule = dynamic_cast<TObjString*>(ruleObject);
+ if (rule == nullptr) continue;
+ const std::string normalized = NormalizeSchemaRule(rule->GetString().Data());
+ if (normalized.empty()) {
+ error = "The on-disk StreamerInfo contains an invalid schema rule";
+ return false;
+ }
+ snapshot.schemaRules.insert(normalized);
+ }
+ }
+ if (!resolve) return true;
+
+ // BuildCheck transfers surviving descriptors into ROOT's global registry.
+ // Aliases and normalized implementation descriptors instead get kCanDelete,
+ // exactly as in TFile::ReadStreamerInfo. Inventory before this mutation.
+ infos->SetOwner(kFALSE);
+ bool resolved = true;
+ next.Reset();
+ while (TObject* object = next()) {
+ auto* info = dynamic_cast<TStreamerInfo*>(object);
+ if (info == nullptr) {
+ object->SetBit(TObject::kCanDelete);
+ continue;
+ }
+ const StreamerInfoIdentity identity{info->GetName(), info->GetClassVersion(), info->GetCheckSum()};
+ info->BuildCheck(&file);
+ if (info->GetNumber() > 0) {
+ snapshot.numbers.insert(info->GetNumber());
+ const auto* registered =
+ dynamic_cast<TStreamerInfo*>(gROOT->GetListOfStreamerInfo()->At(info->GetNumber()));
+ if (!IsImplementationSchema(identity.name) &&
+ (!registered || identity.name != registered->GetName() ||
+ // Streamer writes the absolute value of ROOT's internal version
+ // (for example ROOT::TIOFeatures uses a negative version).
+ identity.version != std::abs(registered->GetClassVersion()) ||
+ identity.checksum != registered->GetCheckSum())) {
+ error = "Conflicting StreamerInfo for " + identity.name + " version " +
+ std::to_string(identity.version) + " checksum " + std::to_string(identity.checksum);
+ if (registered)
+ error += "; resolved to version " + std::to_string(registered->GetClassVersion()) +
+ " checksum " + std::to_string(registered->GetCheckSum());
+ resolved = false;
+ }
+ } else if (!info->TestBit(TObject::kCanDelete)) {
+ error = "Cannot preserve StreamerInfo " + std::string(info->GetName()) + " version " +
+ std::to_string(info->GetClassVersion()) + " checksum " +
+ std::to_string(info->GetCheckSum());
+ resolved = false;
+ }
+ }
+ infos->Clear();
+ return resolved;
+}
+
+bool IsSubset(const RootSchemaSnapshot& expected, const RootSchemaSnapshot& actual, std::string& error) {
+ for (const auto& info : expected.streamerInfos) {
+ if (actual.streamerInfos.count(info) == 0) {
+ // ROOT deliberately normalizes or omits implementation metadata for
+ // standard-library types when it rewrites StreamerInfo. These are
+ // not user schemas; exact historical identities remain mandatory
+ // for every other class.
+ if (IsImplementationSchema(info.name)) continue;
+ error = "Merged file lost StreamerInfo " + info.name + " version " +
+ std::to_string(info.version) + " checksum " + std::to_string(info.checksum);
+ return false;
+ }
+ }
+ for (const auto& rule : expected.schemaRules) {
+ if (actual.schemaRules.count(rule) == 0) {
+ error = "Merged file lost an on-disk schema-evolution rule";
+ return false;
+ }
+ }
+ return true;
+}
+
+bool ReadContentManifest(TDirectory& directory, const std::string& prefix, RootContentManifest& manifest,
+ std::string& error) {
+ TDirectory::TContext directoryContext(&directory);
+ std::map<std::string, TKey*> newestKeys;
+ TIter nextKey(directory.GetListOfKeys());
+ while (auto* key = dynamic_cast<TKey*>(nextKey())) {
+ auto existing = newestKeys.find(key->GetName());
+ if (existing == newestKeys.end() || key->GetCycle() > existing->second->GetCycle())
+ newestKeys[key->GetName()] = key;
+ }
+
+ for (const auto& [name, key] : newestKeys) {
+ const std::string path = prefix.empty() ? name : prefix + "/" + name;
+ RootContentEntry entry{key->GetClassName(), std::nullopt};
+ TClass* keyClass = TClass::GetClass(key->GetClassName(), kFALSE);
+ const bool isDirectory = keyClass != nullptr && keyClass->InheritsFrom(TDirectory::Class());
+ const bool isTree = keyClass != nullptr && keyClass->InheritsFrom(TTree::Class());
+
+ if (isDirectory || isTree) {
+ std::unique_ptr<TObject> object(key->ReadObj());
+ if (object == nullptr) {
+ error =
+ "Cannot read " + std::string(isDirectory ? "directory" : "tree") + " key '" + path + "'";
+ return false;
+ }
+ if (isDirectory) {
+ auto* child = dynamic_cast<TDirectory*>(object.get());
+ if (child == nullptr) {
+ error = "Key '" + path + "' claims directory class '" + entry.className +
+ "' but ROOT read a different object";
+ return false;
+ }
+ manifest[path] = entry;
+ if (!ReadContentManifest(*child, path, manifest, error)) return false;
+ continue;
+ }
+
+ auto* tree = dynamic_cast<TTree*>(object.get());
+ if (tree == nullptr) {
+ error = "Key '" + path + "' claims tree class '" + entry.className +
+ "' but ROOT read a different object";
+ return false;
+ }
+ entry.treeEntries = tree->GetEntries();
+ }
+ manifest[path] = entry;
+ }
+ return true;
+}
+
+bool AddContentManifest(const RootContentManifest& source, RootContentManifest& expected,
+ std::string& error) {
+ for (const auto& [path, sourceEntry] : source) {
+ const auto existing = expected.find(path);
+ if (existing == expected.end()) {
+ expected[path] = sourceEntry;
+ continue;
+ }
+ if (existing->second.className != sourceEntry.className) {
+ error = "ROOT merge input key '" + path + "' has incompatible classes '" +
+ existing->second.className + "' and '" + sourceEntry.className + "'";
+ return false;
+ }
+ if (existing->second.treeEntries.has_value() != sourceEntry.treeEntries.has_value()) {
+ error = "ROOT merge input key '" + path + "' is inconsistently identified as a TTree";
+ return false;
+ }
+ if (sourceEntry.treeEntries.has_value()) {
+ if (*sourceEntry.treeEntries >
+ std::numeric_limits<Long64_t>::max() - *existing->second.treeEntries) {
+ error = "Merged TTree entry count overflows for key '" + path + "'";
+ return false;
+ }
+ *existing->second.treeEntries += *sourceEntry.treeEntries;
+ }
+ }
+ return true;
+}
+
+bool ValidateContentManifest(const RootContentManifest& expected, const RootContentManifest& actual,
+ std::string& error) {
+ for (const auto& [path, expectedEntry] : expected) {
+ const auto found = actual.find(path);
+ if (found == actual.end()) {
+ error = "Merged file lost key '" + path + "'";
+ return false;
+ }
+ if (found->second.className != expectedEntry.className) {
+ error = "Merged key '" + path + "' changed class from '" + expectedEntry.className + "' to '" +
+ found->second.className + "'";
+ return false;
+ }
+ if (expectedEntry.treeEntries.has_value()) {
+ if (!found->second.treeEntries.has_value()) {
+ error = "Merged key '" + path + "' is no longer readable as a TTree";
+ return false;
+ }
+ if (*found->second.treeEntries != *expectedEntry.treeEntries) {
+ error = "Merged TTree '" + path + "' has " + std::to_string(*found->second.treeEntries) +
+ " entries; expected " + std::to_string(*expectedEntry.treeEntries);
+ return false;
+ }
+ }
+ }
+ return true;
+}
+
+bool LoadOnDiskSchemaRules(const RootSchemaSnapshot& snapshot, std::string& error) {
+ for (const auto& text : snapshot.schemaRules) {
+ auto rule = std::make_unique<ROOT::TSchemaRule>();
+ if (!rule->SetFromRule(text.c_str())) {
+ error = "Cannot parse an on-disk schema-evolution rule";
+ return false;
+ }
+ const std::string target = rule->GetTargetClass();
+ TClass* cl = TClass::GetClass(target.c_str(), kFALSE);
+ if (!cl) {
+ error = "Cannot register schema rules for unloaded class " + target;
+ return false;
+ }
+ auto registered = [&]() {
+ if (!cl->GetSchemaRules()) return false;
+ TIter next(cl->GetSchemaRules()->GetRules());
+ while (auto* candidate = dynamic_cast<ROOT::TSchemaRule*>(next())) {
+ TString serialized;
+ candidate->AsString(serialized);
+ if (text == serialized.Data()) return true;
+ }
+ return false;
+ };
+ if (registered()) continue;
+ bool added;
+ if (cl->TestBit(TClass::kIsEmulation)) {
+ auto* rules = cl->GetSchemaRules(kTRUE);
+ added = rules && rules->AddRule(rule.get(), ROOT::Detail::TSchemaRuleSet::kNoCheck);
+ if (added) rule.release();
+ } else {
+ added = TClass::AddRule(text.c_str());
+ }
+ if (!added || !registered()) {
+ error = "Cannot preserve an on-disk schema rule for class " + target;
+ return false;
+ }
+ }
+ return true;
+}
+
+bool ApplySchemaUpdate(TFile& file, const std::set<Int_t>& numbers, std::string& error) {
+ TArrayC* classIndex = file.GetClassIndex();
+ if (classIndex == nullptr) {
+ error = "ROOT file has no StreamerInfo class index";
+ return false;
+ }
+ if (!numbers.empty() && *numbers.rbegin() >= classIndex->GetSize())
+ classIndex->Set(*numbers.rbegin() + 1);
+ for (const Int_t number : numbers) classIndex->fArray[number] = 1;
+ classIndex->fArray[0] = 1;
+ return true;
+}
+
+// Internal operations throw to unwind file owners before cleaning up a failed
+// transaction. The public API translates failures back to bool plus Error().
+void Require(bool success, const std::string& error) {
+ if (!success) throw std::runtime_error(error);
+}
+
+std::filesystem::path UniqueSiblingPath(const std::filesystem::path& target, const std::string& label) {
+ for (int attempt = 0; attempt < 20; ++attempt) {
+ TString uuid = TUUID().AsString();
+ uuid.ReplaceAll("-", "");
+ auto candidate =
+ target.parent_path() / ("." + target.filename().string() + ".rest-" + label + "-" + uuid.Data());
+ if (!std::filesystem::exists(candidate)) return candidate;
+ }
+ return {};
+}
+
+bool ReplaceLocalFile(const std::filesystem::path& temporary, const std::filesystem::path& target,
+ const std::filesystem::path& backup, std::string& error) {
+ const bool targetExists = std::filesystem::exists(target);
+#ifdef WIN32
+ if (targetExists) {
+ if (!ReplaceFileW(target.wstring().c_str(), temporary.wstring().c_str(), backup.wstring().c_str(),
+ REPLACEFILE_WRITE_THROUGH, nullptr, nullptr)) {
+ error = "Windows ReplaceFile failed with error " + std::to_string(GetLastError());
+ return false;
+ }
+ } else if (!MoveFileExW(temporary.wstring().c_str(), target.wstring().c_str(), MOVEFILE_WRITE_THROUGH)) {
+ error = "Windows MoveFileEx failed with error " + std::to_string(GetLastError());
+ return false;
+ }
+#else
+ if (targetExists) {
+ std::error_code ec;
+ std::filesystem::create_hard_link(target, backup, ec);
+ if (ec) {
+ ec.clear();
+ std::filesystem::copy_file(target, backup, std::filesystem::copy_options::none, ec);
+ if (ec) {
+ error = "Cannot create rollback backup " + backup.string() + ": " + ec.message();
+ return false;
+ }
+ }
+ }
+ if (::rename(temporary.c_str(), target.c_str()) != 0) {
+ error = "Atomic rename to " + target.string() + " failed: " + std::strerror(errno);
+ if (targetExists) {
+ std::error_code cleanupError;
+ std::filesystem::remove(backup, cleanupError);
+ if (cleanupError)
+ error += ". REST also could not remove rollback backup '" + backup.string() +
+ "': " + cleanupError.message();
+ }
+ return false;
+ }
+#endif
+ return true;
+}
+
+bool RollBackLocalFile(const std::filesystem::path& target, const std::filesystem::path& backup,
+ bool targetPreviouslyExisted, std::string& error) {
+ if (!targetPreviouslyExisted) {
+ std::error_code ec;
+ std::filesystem::remove(target, ec);
+ if (ec) error = "Cannot remove invalid replacement " + target.string() + ": " + ec.message();
+ return !ec;
+ }
+#ifdef WIN32
+ if (!ReplaceFileW(target.wstring().c_str(), backup.wstring().c_str(), nullptr, REPLACEFILE_WRITE_THROUGH,
+ nullptr, nullptr)) {
+ error = "Cannot roll back " + target.string() + "; Windows error " + std::to_string(GetLastError());
+ return false;
+ }
+#else
+ if (::rename(backup.c_str(), target.c_str()) != 0) {
+ error = "Cannot roll back " + target.string() + ": " + std::strerror(errno);
+ return false;
+ }
+#endif
+ return true;
+}
+} // namespace
+
+TRestRootFileHandle TRestRootFileHandle::Open(const std::string& filename, TRestRootFileMode mode) {
+ TRestRootFileHandle result;
+ if (mode != TRestRootFileMode::Read && TRestTools::IsRemoteRootPath(filename)) {
+ result.fError = "REST refuses to mutate remote ROOT destination '" + filename +
+ "'. Choose an explicit local output file.";
+ return result;
+ }
+
+ const char* option = mode == TRestRootFileMode::Recreate ? "RECREATE" : "READ";
+ bool createForUpdate = false;
+ if (mode == TRestRootFileMode::Update) {
+ std::error_code ec;
+ const auto path = LocalRootPath(filename);
+ if (!path) {
+ result.fError = "Cannot resolve local UPDATE destination '" + filename + "'";
+ return result;
+ }
+ createForUpdate = !std::filesystem::exists(*path, ec);
+ if (ec) {
+ result.fError = "Cannot inspect ROOT destination '" + filename + "': " + ec.message();
+ return result;
+ }
+ // ROOT UPDATE creates missing files. CREATE preserves that contract
+ // without overwriting a file appearing between this check and Open.
+ if (createForUpdate) option = "CREATE";
+ }
+ result.fFile.reset(TFile::Open(filename.c_str(), option));
+ if (result.fFile == nullptr || !result.fFile->IsOpen() || result.fFile->IsZombie()) {
+ result.fError =
+ "Cannot open ROOT file '" + filename +
+ (mode == TRestRootFileMode::Update && !createForUpdate ? "' for read-only UPDATE preflight"
+ : "' in mode " + std::string(option));
+ result.fFile.reset();
+ return result;
+ }
+ if ((mode == TRestRootFileMode::Recreate || createForUpdate) && !result.fFile->IsWritable()) {
+ result.fError = "ROOT file '" + filename + "' is not writable";
+ result.fFile.reset();
+ return result;
+ }
+
+ if (mode == TRestRootFileMode::Update && !createForUpdate &&
+ !PrepareBorrowedUpdate(*result.fFile, &result.fError)) {
+ result.fFile.reset();
+ }
+ return result;
+}
+
+TRestRootFileHandle::TRestRootFileHandle(TRestRootFileHandle&& other) noexcept
+ : fFile(std::move(other.fFile)), fError(std::move(other.fError)) {}
+
+TRestRootFileHandle& TRestRootFileHandle::operator=(TRestRootFileHandle&& other) noexcept {
+ if (this == &other) return *this;
+ Close();
+ fFile = std::move(other.fFile);
+ fError = std::move(other.fError);
+ return *this;
+}
+
+TRestRootFileHandle::~TRestRootFileHandle() { Close(); }
+
+bool TRestRootFileHandle::Close() noexcept {
+ if (fFile == nullptr) return fError.empty();
+ if (fFile->IsOpen()) fFile->Close();
+ const bool success = !fFile->IsZombie() && !fFile->TestBit(TFile::kWriteError);
+ if (!success && fError.empty())
+ fError = "ROOT reported a write error while closing " + std::string(fFile->GetName());
+ fFile.reset();
+ return success;
+}
+
+bool TRestRootFileHandle::PrepareBorrowedUpdate(TFile& file, std::string* error) {
+ bool transitionAttempted = false;
+ try {
+ Require(file.IsOpen() && !file.IsZombie() && !file.IsWritable(),
+ "UPDATE preparation requires a valid TFile that is currently open in READ mode");
+ const std::string filename = file.GetName();
+ const auto path = LocalRootPath(filename);
+ Require(path.has_value(), "REST refuses to mutate remote ROOT destination '" + filename +
+ "'. Choose an explicit local output file.");
+ std::string detail;
+ const auto before = CaptureLocalFileIdentity(*path, detail);
+ Require(before.has_value(), detail);
+ RootSchemaSnapshot schema;
+ Require(ReadSchemaSnapshot(file, schema, detail, true) && LoadOnDiskSchemaRules(schema, detail),
+ detail);
+#ifdef REST_TESTING_ENABLED
+ Require(!gForceNextRootUpdatePreflightFailure.exchange(false),
+ "Forced writable ROOT preflight failure for testing");
+#endif
+ transitionAttempted = true;
+ Require(file.ReOpen("UPDATE") == 0 && file.IsWritable(),
+ "ROOT could not transition '" + filename + "' from validated READ to UPDATE");
+ const auto after = CaptureLocalFileIdentity(*path, detail);
+ Require(after.has_value(), detail);
+ Require(*before == *after,
+ "ROOT file '" + filename + "' changed during the READ-to-UPDATE transition");
+ Require(ApplySchemaUpdate(file, schema.numbers, detail), detail);
+ if (error) error->clear();
+ return true;
+ } catch (const std::exception& failure) {
+ if (transitionAttempted) file.SetWritable(kFALSE);
+ if (error) *error = failure.what();
+ return false;
+ }
+}
+
///////////////////////////////////////////////
/// \brief Returns all the options in an option string
///
Hunk 4@@ -774,6 +1338,173 @@ bool TRestTools::isURL(const string& s) {
@@ -774,6 +1338,173 @@ bool TRestTools::isURL(const string& s) {
return std::regex_match(s, pattern);
}
+///////////////////////////////////////////////
+/// \brief Returns true when ROOT resolves **filename** to a non-local protocol.
+///
+bool TRestTools::IsRemoteRootPath(const std::string& filename) {
+ TUrl url(filename.c_str(), kTRUE);
+ return url.IsValid() && std::string(url.GetProtocol()) != "file";
+}
+
+#ifdef REST_TESTING_ENABLED
+///////////////////////////////////////////////
+/// \brief Forces one writable ROOT preflight failure for no-mutation tests.
+///
+void TRestTools::ForceNextRootUpdatePreflightFailureForTesting() {
+ gForceNextRootUpdatePreflightFailure.store(true);
+}
+
+///////////////////////////////////////////////
+/// \brief Forces one post-replacement validation failure for rollback tests.
+///
+void TRestTools::ForceNextTransactionalMergeValidationFailureForTesting() {
+ gForceNextMergeValidationFailure.store(true);
+}
+#endif
+
+///////////////////////////////////////////////
+/// \brief Transactionally merges ROOT files into a local destination.
+///
+bool TRestTools::MergeRootFilesTransactionally(const std::string& outputFile,
+ const std::vector<std::string>& inputFiles,
+ const std::string& existingTarget, bool removeInputsOnSuccess,
+ std::string* error) {
+ namespace fs = std::filesystem;
+ fs::path temporary;
+ try {
+ const auto localOutput = LocalRootPath(outputFile);
+ Require(localOutput.has_value(), "REST refuses to replace remote ROOT destination '" + outputFile +
+ "'. Choose an explicit local output file.");
+ const fs::path target = fs::weakly_canonical(fs::absolute(*localOutput));
+ Require(fs::is_directory(target.parent_path()),
+ "Output directory does not exist for '" + target.string() + "'");
+
+ std::vector<std::string> sources;
+ if (!existingTarget.empty()) sources.push_back(existingTarget);
+ sources.insert(sources.end(), inputFiles.begin(), inputFiles.end());
+ Require(!sources.empty(), "No ROOT input files were provided for transactional merge");
+
+ RootSchemaSnapshot schema;
+ RootContentManifest content, workers;
+ std::string detail;
+ for (std::size_t index = 0; index < sources.size(); ++index) {
+ auto source = TRestRootFileHandle::Open(sources[index], TRestRootFileMode::Read);
+ Require(bool(source), source.Error());
+ RootContentManifest incoming;
+ if (!ReadSchemaSnapshot(*source, schema, detail, true) ||
+ !LoadOnDiskSchemaRules(schema, detail) ||
+ !ReadContentManifest(*source, "", incoming, detail) ||
+ !AddContentManifest(incoming, !existingTarget.empty() && index == 0 ? content : workers,
+ detail))
+ throw std::runtime_error("Cannot prepare merge input '" + sources[index] + "': " + detail);
+ }
+ // ROOT UPDATE merges workers together, replacing same-named target
+ // objects; target-only objects stay in the byte-for-byte seed untouched.
+ for (const auto& [path, incoming] : workers) {
+ const auto found = content.find(path);
+ Require(found == content.end() || found->second.className == incoming.className,
+ "ROOT merge input key '" + path + "' has incompatible classes");
+ content[path] = incoming;
+ }
+
+ temporary = UniqueSiblingPath(target, "merge");
+ const auto backup = UniqueSiblingPath(target, "backup");
+ Require(!temporary.empty() && !backup.empty(),
+ "Cannot allocate temporary paths next to '" + target.string() + "'");
+ if (!existingTarget.empty()) {
+ const auto localExisting = LocalRootPath(existingTarget);
+ if (localExisting)
+ fs::copy_file(*localExisting, temporary);
+ else
+ Require(TFile::Cp(existingTarget.c_str(), temporary.c_str(), kFALSE),
+ "ROOT could not seed temporary merge output from '" + existingTarget + "'");
+ }
+ {
+ auto output = TRestRootFileHandle::Open(temporary.string(), existingTarget.empty()
+ ? TRestRootFileMode::Recreate
+ : TRestRootFileMode::Update);
+ Require(bool(output), output.Error());
+ Require(ApplySchemaUpdate(*output, schema.numbers, detail), detail);
+ if (inputFiles.empty()) {
+ Require(output.Close(), output.Error());
+ } else {
+ TFileMerger merger(kFALSE);
+ merger.SetPrintLevel(0);
+ Require(merger.OutputFile(std::move(output.fFile)),
+ "ROOT could not adopt temporary merge output '" + temporary.string() + "'");
+ for (const auto& input : inputFiles)
+ Require(merger.AddFile(input.c_str(), kFALSE),
+ "ROOT could not add merge input '" + input + "'");
+ Require(merger.Merge(), "ROOT failed while merging into '" + temporary.string() + "'");
+ }
+ }
+
+ auto validate = [&](const fs::path& path) {
+ auto file = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Read);
+ Require(bool(file), file.Error());
+ RootSchemaSnapshot actualSchema;
+ RootContentManifest actualContent;
+ Require(ReadSchemaSnapshot(*file, actualSchema, detail) &&
+ IsSubset(schema, actualSchema, detail) &&
+ ReadContentManifest(*file, "", actualContent, detail) &&
+ ValidateContentManifest(content, actualContent, detail),
+ detail);
+ };
+ validate(temporary);
+ const bool targetExisted = fs::exists(target);
+ if (targetExisted) fs::permissions(temporary, fs::status(target).permissions());
+ Require(ReplaceLocalFile(temporary, target, backup, detail), detail);
+ try {
+ validate(target);
+#ifdef REST_TESTING_ENABLED
+ Require(!gForceNextMergeValidationFailure.exchange(false),
+ "Forced post-replacement ROOT validation failure for testing");
+#endif
+ } catch (const std::exception& failure) {
+ std::string rollbackError;
+ detail = failure.what();
+ if (!RollBackLocalFile(target, backup, targetExisted, rollbackError))
+ detail += ". Rollback also failed: " + rollbackError + ". The backup is at '" +
+ backup.string() + "'";
+ throw std::runtime_error(detail);
+ }
+
+ if (targetExisted) {
+ std::error_code ec;
+ fs::remove(backup, ec);
+ Require(!ec, "Merged output is valid, but REST could not remove rollback backup '" +
+ backup.string() + "': " + ec.message());
+ }
+ if (removeInputsOnSuccess) {
+ std::string failures;
+ for (const auto& input : inputFiles) {
+ const auto localInput = LocalRootPath(input);
+ if (!localInput) continue;
+ std::error_code ec;
+ const auto absoluteInput = fs::weakly_canonical(fs::absolute(*localInput), ec);
+ if (!ec && absoluteInput == target) continue;
+ if (!ec) fs::remove(*localInput, ec);
+ if (ec) failures += "'" + input + "': " + ec.message() + "; ";
+ }
+ Require(failures.empty(),
+ "Merged output is valid, but REST could not remove input file(s): " + failures);
+ }
+ if (error) error->clear();
+ return true;
+ } catch (const std::exception& failure) {
+ // All ROOT owners have unwound before unlinking the candidate, including
+ // failures in TFileMerger adoption, schema registration or filesystem APIs.
+ std::string detail = failure.what();
+ if (!temporary.empty()) {
+ std::error_code ec;
+ std::filesystem::remove(temporary, ec);
+ if (ec) detail += ". Cannot remove temporary file '" + temporary.string() + "': " + ec.message();
+ }
+ if (error) *error = detail;
+ return false;
+ }
+}
+
///////////////////////////////////////////////
/// \brief Returns true if the **path** given by argument is writable
///
Implementation history
Show development rationale and earlier-revision diff (optional)
This background is not needed to review the current proposal. Before human review, #567 was reimplemented internally to simplify schema processing and cleanup while retaining the public interface and preservation policy. During development this was called the “compact” version.
The historical comparison starts at 6077128fe9edd582d6fa3dcfac5ff07d45152df7, an earlier revision of this same PR—not master. Commit c6bdebfd contains the rewrite; 65d83f6f adds the final clarity improvements; 06cddd32 removes an unused helper output. Commit 3fec0db1 adds the visible user-macro migration warning and tutorial. The separate 8f5bad5e commit contains validation notes and tooling.
| Change | Reason |
|---|---|
| One schema inventory/resolution pass | Record on-disk identity before BuildCheck mutates descriptors; retain ROOT's ownership transfer while avoiding duplicate reads/state. |
| One per-rule registration path | Reduce repeated parsing/maps while still checking that embedded rules are retained for loaded or emulated classes. |
| Shared validator + exception cleanup | Remove repeated close/cleanup branches; release owners before deleting a failed candidate. Keep rollback inside the post-install failure boundary. |
| Three substantive review fixes | Reject wrong same-version checksum resolution; keep create-if-missing UPDATE/GainMap export; never delete output through an input-path alias. |
| Three final improvements | Remove nine redundant TRestRun close lines; use explicit createForUpdate state and accurate path/CREATE errors; retain failing merge input filename in preflight diagnostics. |
Production size vs original #567: 192 fewer physical lines across the changed production files, including comments/blank lines. The core rewrite before finishing changes was −187 lines (affected I/O sections 877 → 688). Added regression tests and documentation are counted separately; no original tests were removed.
An independent review also produced an incremental patch to the earlier implementation. Its substantive fixes were incorporated into the current proposal; that alternative is not a prerequisite for this review.
Diff against the earlier PR revision
These 12 files show only the rewrite and follow-up changes, including the user-macro migration documentation. The main review diff above compares the whole feature against master. Each historical file and hunk can be expanded independently.
01CONTRIBUTING.mdChanges since the earlier PR revisiondeveloper docsrisk: lowimpact: medium+5 / −0
- Intent
- Document the supported developer entry point, modes, ownership, checked closes, merge behavior and limitations; link it from the contribution guide.
- Invariant
- Preservation is not schema repair, and transactional replacement is not a durability or writer-locking promise.
- Human review
- Verify examples encourage checked Close(), and clarify remote-write rejection and intentionally non-copyable TRestRun.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ab52890e..9bfcc4e5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md
Hunk 1@@ -28,6 +28,11 @@ Code that creates, updates, replaces, or merges ROOT files must follow the
@@ -28,6 +28,11 @@ Code that creates, updates, replaces, or merges ROOT files must follow the [safe writable ROOT I/O guide](doc/developer/Safe%20writable%20ROOT%20IO.md). It documents the required schema preflight, ownership, transactional replacement, and local/remote path rules. +This also applies to **existing library code and user-written macros**, not only new code. The handle does not +intercept direct `TFile` calls: audit and migrate remaining UPDATE opens, including constructors and `ReOpen`. +The [user macro migration guide](doc/tutorials/Updating%20ROOT%20files%20from%20macros.md) provides a before/after +example. A passing framework test suite does not establish that external macros have been migrated. + ### Pipeline validation tests TODO : Explain how pipeline validation tests should be implemented
02README.mdChanges since the earlier PR revisionuser docsrisk: lowimpact: high+17 / −2
- Intent
- Make existing user-macro migration visible: warn about direct UPDATE calls, provide a checked-write example, and explain borrowed pointers, close errors and refused updates.
- Invariant
- Installing REST does not intercept direct TFile calls. Preservation is neither legacy recovery nor rollback for an entire macro.
- Human review
- Check that existing private macros are explicitly in scope, read-only macros are distinguished, and the example never bypasses failed preflight.
diff --git a/README.md b/README.md index d871de18..b8f90a36 100644 --- a/README.md +++ b/README.md
Hunk 1@@ -8,6 +8,19 @@
@@ -8,6 +8,19 @@ The REST-for-Physics (Rare Event Searches Toolkit) Framework is mainly written in C++ and it is fully integrated with [ROOT](https://root.cern.ch) I/O interface. REST was initially born as a collaborative software effort to provide common tools for acquisition, simulation, and data analysis of gaseous Time Projection Chambers (TPCs). However, the framework is already extending its usage to be non-exclusive of detector data analysis. The possibilities of the framework are provided by the different libraries and packages written for REST in our community. +## Important: review existing macros that update ROOT files + +If your macro adds histograms, changes metadata, or otherwise modifies an existing ROOT file, replace direct +`TFile` UPDATE opens with `TRestRootFileHandle`. This applies to **existing user macros and library code**, not +just new development. Direct ROOT calls are not automatically protected by REST and can still lose historical +schema information, even when the macro only writes a histogram. Read-only macros do not need this migration. + +See [Updating ROOT files from macros](doc/tutorials/Updating%20ROOT%20files%20from%20macros.md) for a migration +example, ownership rules, and what to do if an update is refused. The new interface addresses an existing risk; +it does not make previously written macros newly unsafe. + +## Framework overview + The REST Framework provides 3 interfaces that prototype the use of **event types**, **metadata** and **event processes** through `TRestEvent`, `TRestMetadata` and `TRestEventProcess` abstract class definitions. Any REST library will implement **specific objects** that inherit from those 3 basic interfaces.
Hunk 2@@ -63,8 +76,10 @@ Any **metadata** object written with REST **will be stamped** with few metadata
@@ -63,8 +76,10 @@ Any **metadata** object written with REST **will be stamped** with few metadata If different REST versions were used to write a ROOT file, e.g. at different steps of the data processing chain, the historic metadata objects will preserve their original version. However, the `TRestRun` metadata object **will always store** the version used to write the ROOT file. -After REST release 2.2.1., REST implements correctly the `ROOT schema evolution`. Therefore, any new REST version should always be backwards compatible. -I.e. Any file written after v2.2.1 should be readable without problems with any future version. +REST uses ROOT schema evolution to support reading historical data. Compatibility depends on usable historical +schema information and compatible class definitions/evolution rules; it is not guaranteed by a file's release +number alone. Use the checked writable interface described above to preserve existing schema information. +Files already missing required information may need separate recovery. A major change at 2.3 will prevent from backwards compatibility, since class names have been reviewed.
03doc/developer/Safe writable ROOT IO.mdChanges since the earlier PR revisiondeveloper docsrisk: lowimpact: medium+31 / −6
- Intent
- Document the supported developer entry point, modes, ownership, checked closes, merge behavior and limitations; link it from the contribution guide.
- Invariant
- Preservation is not schema repair, and transactional replacement is not a durability or writer-locking promise.
- Human review
- Verify examples encourage checked Close(), and clarify remote-write rejection and intentionally non-copyable TRestRun.
diff --git a/doc/developer/Safe writable ROOT IO.md b/doc/developer/Safe writable ROOT IO.md index e77fca72..5067729c 100644 --- a/doc/developer/Safe writable ROOT IO.md +++ b/doc/developer/Safe writable ROOT IO.md
Hunk 1@@ -5,9 +5,24 @@ a file directly with `TFile::Open(..., "UPDATE")` bypasses REST's schema preflig
@@ -5,9 +5,24 @@ a file directly with `TFile::Open(..., "UPDATE")` bypasses REST's schema preflig schema metadata before REST has established that every historical class layout is usable. Framework code that creates or mutates ROOT files must therefore use `TRestRootFileHandle`. +## Existing code and user macros need attention too + +**This is not only a convention for new code.** Existing framework/library code and user-written macros that +open ROOT files directly for UPDATE still bypass the protection. The handle does not intercept `TFile::Open`, +`TFile` constructors, or `ReOpen`; installing a newer REST version does not redirect those calls automatically. +Even an update that only adds a histogram can rewrite file-level schema metadata. Whether information is lost +depends on the file's historical schemas and ROOT's handling of them; this change addresses a pre-existing risk. + +Audit existing writable opens and migrate them to the handle, or use the borrowed-file adapter below where +ownership cannot be changed. The adapter must receive a READ-mode file, not one already opened for UPDATE. +Read-only opens do not need migration for this schema-preservation protection. See the +[user-facing migration example](../tutorials/Updating%20ROOT%20files%20from%20macros.md), which also covers error +handling, borrowed pointers, and existing-file safety. Do not assume all library or external macros are covered +merely because framework CI passes. + ## Opening files -Use `TRestRootFileHandle::Open` for new code: +Use `TRestRootFileHandle::Open` for new code and when migrating existing writable opens: ```cpp #include "TRestTools.h"
Hunk 2@@ -28,13 +43,17 @@ if (!file.Close()) {
@@ -28,13 +43,17 @@ if (!file.Close()) {
```
The available modes are `Read`, `Recreate`, and `Update`. `Recreate` intentionally replaces an existing file
-and must not be used as a shortcut for `Update`. An update is initially opened read-only. REST inventories the
+and must not be used as a shortcut for `Update`. Like ROOT's UPDATE mode, `Update` creates a missing local
+file; REST uses CREATE for this case so that a concurrently appearing file cannot be overwritten.
+An existing file is initially opened read-only. REST inventories the
exact class-name, class-version, and checksum tuples stored in the file, collects the embedded schema rules,
asks ROOT to resolve the on-disk entries into loaded or emulated classes, and only then registers the embedded
-rules. This follows ROOT's own `TFile::ReadStreamerInfo`/`TStreamerInfo::BuildCheck` ownership and resolution
+rules. Inventory and resolution share one read of the on-disk schema record. REST checks that each resolved
+user schema still has its original name, version, and checksum; a conflicting cached layout is rejected before
+the file becomes writable. This follows ROOT's own `TFile::ReadStreamerInfo`/`TStreamerInfo::BuildCheck` ownership and resolution
rules, including unloaded classes and entries such as `ROOT::TIOFeatures`. REST then transitions the same
-`TFile` to update mode and verifies the local file identity and ROOT UUID before marking the historical
-class-index entries required for writing.
+`TFile` to update mode and checks that its filesystem identity still matches the identity captured before
+preflight. It then marks the historical class-index entries required for writing.
`PrepareBorrowedUpdate(TFile&, std::string*)` provides the same update preparation when legacy code already
owns a `TFile`. The supplied file must be valid, open in `READ` mode, and not writable:
Hunk 3@@ -95,7 +114,8 @@ result in a temporary sibling of the local destination. Before replacement it va
@@ -95,7 +114,8 @@ result in a temporary sibling of the local destination. Before replacement it va StreamerInfos and schema rules, recursive key paths and classes, and `TTree` entry counts using ROOT's UPDATE semantics (summed across new inputs, replacing a same-named target tree). It validates the installed file again and attempts to restore the previous destination from a rollback backup on failure. -Local input files are removed only after successful replacement and validation when +Destination paths and cleanup exclusions resolve filesystem aliases, so using `./` or a symbolic link cannot +cause input cleanup to delete the completed output. Local input files are removed only after successful replacement and validation when `removeInputsOnSuccess` is true. A false return can also mean that the merged output is valid but a backup or input could not be removed.
Hunk 4@@ -104,6 +124,11 @@ operations on sibling paths, but this is not a promise of power-loss durability
@@ -104,6 +124,11 @@ operations on sibling paths, but this is not a promise of power-loss durability mounted filesystem. Rollback can itself fail; preserve and report the detailed error, including any retained backup path. +Internally, the update and merge operations translate failures into exceptions to unwind ROOT file owners +before cleanup. Their public interfaces retain boolean results and error strings. Both temporary and installed +merge results pass the same schema/content validator, and any exception during installed-result validation +triggers rollback before the failure is returned. + ## Local and remote paths Remote ROOT files may be read and may be merge inputs if the installed ROOT transports can open them. Writable
04doc/tutorials/Updating ROOT files from macros.mdChanges since the earlier PR revisionuser docsrisk: lowimpact: high+87 / −0
- Intent
- Make existing user-macro migration visible: warn about direct UPDATE calls, provide a checked-write example, and explain borrowed pointers, close errors and refused updates.
- Invariant
- Installing REST does not intercept direct TFile calls. Preservation is neither legacy recovery nor rollback for an entire macro.
- Human review
- Check that existing private macros are explicitly in scope, read-only macros are distinguished, and the example never bypasses failed preflight.
diff --git a/doc/tutorials/Updating ROOT files from macros.md b/doc/tutorials/Updating ROOT files from macros.md new file mode 100644 index 00000000..b5c4cb96 --- /dev/null +++ b/doc/tutorials/Updating ROOT files from macros.md
Hunk 1@@ -0,0 +1,87 @@
@@ -0,0 +1,87 @@
+# Updating ROOT files from macros
+
+**Review your existing macros as well as new ones.** If a macro adds histograms, writes a readout, changes
+metadata, or otherwise modifies an existing ROOT file, use `TRestRootFileHandle` instead of a direct `TFile`
+UPDATE open. This applies to personal macros outside the REST repositories too.
+
+ROOT files can contain historical class descriptions (`StreamerInfo`) and schema-evolution rules needed to
+read their data. A direct UPDATE open can allow ROOT to rewrite this metadata without REST's preservation
+checks, even if your macro only adds a histogram. Not every UPDATE loses information, but successful execution
+alone does not prove historical data remains readable.
+
+This is a pre-existing risk, not a new incompatibility introduced by the handle. Updating REST protects code
+that uses the checked interface; it does **not** automatically redirect direct ROOT calls in your macros.
+Macros that only read files do not need this migration for schema preservation.
+
+## What to look for
+
+Look for `TFile::Open(..., "UPDATE")`, stack/heap `TFile` constructors using UPDATE (including lowercase
+`"update"`), and calls to `ReOpen("UPDATE")`. Check Python/PyROOT macros and helper functions too. For example:
+
+```sh
+rg -n -i 'update|reopen' --glob '*.{C,cxx,cpp,h,py}' path/to/your/macros
+```
+
+This is only a starting point: inspect mode variables and wrapper functions manually. A search or CI check is
+not proof that every writable open is safe, and repository CI cannot inspect private macros on your machine.
+
+## Replace the open, preserve the write, check the close
+
+The old pattern bypasses REST's preflight:
+
+```cpp
+TFile* file = TFile::Open(filename, "UPDATE");
+// Write histograms, metadata, etc.
+file->Close();
+delete file;
+```
+
+Use a REST build that provides `TRestRootFileHandle`, with REST loaded in your macro environment. This complete
+example adds a small metadata note; use the same structure around your own histogram or metadata writes:
+
+```cpp
+#include <TFile.h>
+#include <TNamed.h>
+#include <iostream>
+
+#include "TRestTools.h"
+
+bool AddAnalysisNote(const char* filename) {
+ auto file = TRestRootFileHandle::Open(filename, TRestRootFileMode::Update);
+ if (!file) {
+ std::cerr << file.Error() << '\n';
+ return false;
+ }
+
+ file->cd();
+ TNamed note("analysisNote", "Updated with checked REST ROOT I/O");
+ const bool written = note.Write() > 0;
+ const bool closed = file.Close();
+ if (!written) std::cerr << "Could not write analysisNote\n";
+ if (!closed) std::cerr << file.Error() << '\n';
+ return written && closed;
+}
+```
+
+The handle owns the file and closes it automatically on destruction, but writers must explicitly check
+`Close()` to report errors. Pass `file.Get()` to APIs expecting a `TFile*`; that pointer is borrowed. Do not
+delete it or use it, or file-owned objects, after the handle closes. Do not retain the old `delete file` line.
+
+Like ROOT UPDATE, `Update` creates a missing local file. If your macro requires an existing input, keep that
+existence check. `Recreate` intentionally replaces existing contents: never substitute it for an UPDATE that
+failed. Writable destinations must be local; remote reads remain subject to ROOT's available transports.
+
+## If the update is refused
+
+Report the error and stop; do not fall back to direct ROOT UPDATE or change the mode to RECREATE. Consult the
+error before deciding whether a compatible dictionary, corrected schema rule, or separate legacy recovery is
+needed. The handle preserves usable schema information already present; it does not reconstruct missing
+StreamerInfo or repair incompatible evolution rules.
+
+Keep a backup before modifying valuable data. Checked UPDATE is not a rollback transaction for your entire
+macro: failed writes may leave partial changes, and it does not provide concurrent-writer locking or power-loss
+durability. To work on a remote file, explicitly create a local copy and update that copy.
+
+For existing code that must retain ownership of a READ-mode `TFile`, see `PrepareBorrowedUpdate` in the
+[developer guide](../developer/Safe%20writable%20ROOT%20IO.md). The same guide covers transactional merging and
+the intentionally non-copyable `TRestRun` interface.
05source/framework/core/src/TRestRun.cxxChanges since the earlier PR revisionownershiprisk: highimpact: high+0 / −9
- Intent
- Give TRestRun owned input/output handles while retaining raw aliases and its legacy string-mode entry point; route worker merging through the transaction helper.
- Invariant
- No shallow copies of live run ownership. CloseFile clears owners before the replacement paths; redundant later closes are removed.
- Human review
- Follow tree/file destruction order, split-file processing and post-merge metadata writes. Existing macros must not start owning borrowed pointers.
diff --git a/source/framework/core/src/TRestRun.cxx b/source/framework/core/src/TRestRun.cxx index e0794f22..7fa48e58 100644 --- a/source/framework/core/src/TRestRun.cxx +++ b/source/framework/core/src/TRestRun.cxx
Hunk 1@@ -370,10 +370,6 @@ void TRestRun::OpenInputFile(const TString& filename, const string& mode) {
@@ -370,10 +370,6 @@ void TRestRun::OpenInputFile(const TString& filename, const string& mode) {
RESTError << inputFile.Error() << RESTendl;
exit(1);
}
- if (fInputFileOwner && !fInputFileOwner.Close()) {
- RESTError << fInputFileOwner.Error() << RESTendl;
- exit(1);
- }
fInputFileOwner = std::move(inputFile);
fInputFile = fInputFileOwner.Get();
Hunk 2@@ -443,7 +439,6 @@ void TRestRun::OpenInputFile(const TString& filename, const string& mode) {
@@ -443,7 +439,6 @@ void TRestRun::OpenInputFile(const TString& filename, const string& mode) {
ReadInputFileTrees();
}
} else {
- if (fInputFileOwner && !fInputFileOwner.Close()) RESTError << fInputFileOwner.Error() << RESTendl;
fInputFile = nullptr;
fAnalysisTree = nullptr;
if (fFileProcess != nullptr) {
Hunk 3@@ -1080,10 +1075,6 @@ TFile* TRestRun::FormOutputFile() {
@@ -1080,10 +1075,6 @@ TFile* TRestRun::FormOutputFile() {
RESTError << outputFile.Error() << RESTendl;
return nullptr;
}
- if (fOutputFileOwner && !fOutputFileOwner.Close()) {
- RESTError << fOutputFileOwner.Error() << RESTendl;
- return nullptr;
- }
fOutputFileOwner = std::move(outputFile);
fOutputFile = fOutputFileOwner.Get();
fAnalysisTree = new TRestAnalysisTree("AnalysisTree", "AnalysisTree");
06source/framework/test/io/CMakeLists.txtChanges since the earlier PR revisiontestsrisk: mediumimpact: medium+22 / −0
- Intent
- Build isolated old/current dictionary libraries and fixture executables as dependencies of the existing framework test target.
- Invariant
- Test dictionaries must not accidentally share a process or mask missing historical file metadata.
- Human review
- Check dictionary links, fixture authenticity and which reader actually verifies the result.
diff --git a/source/framework/test/io/CMakeLists.txt b/source/framework/test/io/CMakeLists.txt index a3e19a1d..25d27080 100644 --- a/source/framework/test/io/CMakeLists.txt +++ b/source/framework/test/io/CMakeLists.txt
Hunk 1@@ -97,3 +97,25 @@ target_compile_definitions(
@@ -97,3 +97,25 @@ target_compile_definitions(
REST_IO_ANALYSIS_TREE_VERIFIER="$<TARGET_FILE:RestIOAnalysisTreeVerifier>"
REST_PAIR_BASE_SOURCE_FILE="${CMAKE_SOURCE_DIR}/pipeline/fileopening/v2.2.30_hits.root"
)
+
+foreach (layout Old New)
+ set(target "RestIOConflict${layout}")
+ set(dictionary "G__${target}")
+ set(options)
+ if (layout STREQUAL "Old")
+ list(APPEND options -DREST_IO_OLD_LAYOUT)
+ endif ()
+ root_generate_dictionary(
+ ${dictionary} "${CMAKE_CURRENT_SOURCE_DIR}/ConflictPayload.h" LINKDEF
+ "${CMAKE_CURRENT_SOURCE_DIR}/ConflictLinkDef.h" OPTIONS ${options})
+ add_executable(${target} ConflictFixture.cxx
+ "${CMAKE_CURRENT_BINARY_DIR}/${dictionary}.cxx")
+ target_compile_options(${target} PRIVATE ${options})
+ target_link_libraries(${target} PRIVATE RestFramework ${ROOT_LIBRARIES})
+ set_target_properties(${target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+ "${FIXTURE_OUTPUT_DIR}/bin")
+ add_dependencies(testRestFramework ${target})
+ target_compile_definitions(
+ testRestFramework
+ PRIVATE "REST_IO_CONFLICT_${layout}=\"$<TARGET_FILE:${target}>\"")
+endforeach ()
07source/framework/test/io/ConflictFixture.cxxChanges since the earlier PR revisiontestsrisk: mediumimpact: high+33 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/io/ConflictFixture.cxx b/source/framework/test/io/ConflictFixture.cxx new file mode 100644 index 00000000..2fed99da --- /dev/null +++ b/source/framework/test/io/ConflictFixture.cxx
Hunk 1@@ -0,0 +1,33 @@
@@ -0,0 +1,33 @@
+#include <TFile.h>
+#include <TStreamerInfo.h>
+
+#include <iostream>
+#include <string>
+
+#include "ConflictPayload.h"
+#include "TRestTools.h"
+
+ClassImp(TRestIOConflictPayload);
+
+int main(int argc, char** argv) {
+ if (argc != 2 && argc != 3) return 2;
+ // Establish the active dictionary before ROOT reads the conflicting file.
+ TRestIOConflictPayload::Class()->GetStreamerInfo()->Build();
+#ifdef REST_IO_OLD_LAYOUT
+ TFile file(argv[1], "CREATE");
+ if (file.IsZombie()) return 3;
+ TRestIOConflictPayload payload;
+ payload.Write("payload");
+ return 0;
+#else
+ if (argc == 3) {
+ std::string error;
+ const bool merged = TRestTools::MergeRootFilesTransactionally(argv[2], {argv[1]}, "", true, &error);
+ std::cout << "merged=" << merged << " error=" << error << '\n';
+ return merged ? 4 : 0;
+ }
+ auto file = TRestRootFileHandle::Open(argv[1], TRestRootFileMode::Update);
+ std::cout << "update=" << bool(file) << " error=" << file.Error() << '\n';
+ return file ? 4 : 0;
+#endif
+}
08source/framework/test/io/ConflictLinkDef.hChanges since the earlier PR revisiontestsrisk: mediumimpact: high+1 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/io/ConflictLinkDef.h b/source/framework/test/io/ConflictLinkDef.h new file mode 100644 index 00000000..7d8b688b --- /dev/null +++ b/source/framework/test/io/ConflictLinkDef.h
Hunk 1@@ -0,0 +1 @@
@@ -0,0 +1 @@ +#pragma link C++ class TRestIOConflictPayload + ;
09source/framework/test/io/ConflictPayload.hChanges since the earlier PR revisiontestsrisk: mediumimpact: high+12 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/io/ConflictPayload.h b/source/framework/test/io/ConflictPayload.h new file mode 100644 index 00000000..47445375 --- /dev/null +++ b/source/framework/test/io/ConflictPayload.h
Hunk 1@@ -0,0 +1,12 @@
@@ -0,0 +1,12 @@
+#include <TObject.h>
+
+// Deliberately invalid evolution: the layout changed without a version bump.
+class TRestIOConflictPayload : public TObject {
+ public:
+#ifdef REST_IO_OLD_LAYOUT
+ int fValue = 123;
+#else
+ double fValue = 123.5;
+#endif
+ ClassDef(TRestIOConflictPayload, 2);
+};
10source/framework/test/src/RootIO.cxxChanges since the earlier PR revisiontestsrisk: mediumimpact: high+84 / −0
- Intent
- Exercise loaded and unloaded dictionaries, automatic float-to-double evolution, renamed-member rules, historical schemas, real AnalysisTree values, rollback and path policies. Additional regressions cover missing UPDATE destinations (including GainMap::Export), output aliases, new-output rollback, environment expansion and symlinks.
- Invariant
- Failed preflight does not change source bytes; failed publication retains sources and restores/removes output as appropriate.
- Human review
- Inspect the values being asserted, not merely successful opens. Fault injection covers controlled failure, not power-loss durability.
diff --git a/source/framework/test/src/RootIO.cxx b/source/framework/test/src/RootIO.cxx index 47ca9875..10187516 100644 --- a/source/framework/test/src/RootIO.cxx +++ b/source/framework/test/src/RootIO.cxx
Hunk 1@@ -8,10 +8,12 @@
@@ -8,10 +8,12 @@ #include <TNamed.h> #include <TObjString.h> #include <TRestAnalysisTree.h> +#include <TRestDataSetGainMap.h> #include <TRestProcessRunner.h> #include <TRestRun.h> #include <TRestTools.h> #include <TStreamerInfo.h> +#include <TSystem.h> #include <TTree.h> #include <gtest/gtest.h>
Hunk 2@@ -677,3 +679,85 @@ TEST(RootIO, TRestRunRejectsUnsupportedInputModes) {
@@ -677,3 +679,85 @@ TEST(RootIO, TRestRunRejectsUnsupportedInputModes) {
},
::testing::ExitedWithCode(1), ".*");
}
+
+TEST(RootIO, UpdateCreatesMissingFileLikeRootWithoutReplacingExistingData) {
+ TemporaryDirectory temporary;
+ const auto path = temporary.Path() / "new.root";
+ auto file = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Update);
+ ASSERT_TRUE(file) << file.Error();
+ TNamed marker("original", "UPDATE creates a missing file");
+ marker.Write();
+ ASSERT_TRUE(file.Close()) << file.Error();
+ auto updated = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Update);
+ ASSERT_TRUE(updated) << updated.Error();
+ EXPECT_NE(updated->Get<TNamed>("original"), nullptr);
+ EXPECT_TRUE(updated.Close()) << updated.Error();
+
+ const auto gainPath = temporary.Path() / "gain.root";
+ TRestDataSetGainMap gainMap;
+ gainMap.Export(gainPath.string());
+ ASSERT_TRUE(fs::exists(gainPath));
+ EXPECT_TRUE(HasKeyClass(gainPath, "TRestDataSetGainMap"));
+}
+
+TEST(RootIO, MergeCleanupNeverDeletesOutputReferencedByPathAlias) {
+ TemporaryDirectory temporary;
+ const auto path = temporary.Path() / "target.root";
+ CreateNamedInput(path, "survivor");
+ const auto alias = temporary.Path() / "." / "target.root";
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(path.string(), {alias.string()}, "", true, &error))
+ << error;
+ auto file = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Read);
+ ASSERT_TRUE(file) << file.Error();
+ EXPECT_NE(file->Get<TNamed>("survivor"), nullptr);
+}
+
+TEST(RootIO, FailedPublicationOfNewOutputRemovesCandidateAndPreservesInput) {
+ TemporaryDirectory temporary;
+ const auto input = temporary.Path() / "input.root";
+ const auto output = temporary.Path() / "output.root";
+ CreateNamedInput(input, "survivor");
+ const auto before = ReadBytes(input);
+ TRestTools::ForceNextTransactionalMergeValidationFailureForTesting();
+ std::string error;
+ EXPECT_FALSE(
+ TRestTools::MergeRootFilesTransactionally(output.string(), {input.string()}, "", true, &error));
+ EXPECT_NE(error.find("Forced post-replacement"), std::string::npos);
+ EXPECT_FALSE(fs::exists(output));
+ EXPECT_EQ(ReadBytes(input), before);
+ EXPECT_EQ(std::distance(fs::directory_iterator(temporary.Path()), fs::directory_iterator()), 1);
+}
+
+TEST(RootIO, UpdateHonorsRootEnvironmentVariableExpansion) {
+ TemporaryDirectory temporary;
+ const auto path = temporary.Path() / "expanded.root";
+ CreateNamedInput(path, "original");
+ gSystem->Setenv("REST_IO_TEST_DIRECTORY", temporary.Path().c_str());
+ auto file = TRestRootFileHandle::Open("$REST_IO_TEST_DIRECTORY/expanded.root", TRestRootFileMode::Update);
+ gSystem->Unsetenv("REST_IO_TEST_DIRECTORY");
+ ASSERT_TRUE(file) << file.Error();
+ EXPECT_NE(file->Get<TNamed>("original"), nullptr);
+ EXPECT_TRUE(file.Close()) << file.Error();
+}
+
+TEST(RootIO, MergeUpdatesSymbolicLinkDestinationWithoutReplacingTheLink) {
+ TemporaryDirectory temporary;
+ const auto target = temporary.Path() / "target.root";
+ const auto alias = temporary.Path() / "alias.root";
+ const auto input = temporary.Path() / "input.root";
+ CreateNamedInput(target, "original");
+ CreateNamedInput(input, "incoming");
+ std::error_code ec;
+ fs::create_symlink(target, alias, ec);
+ if (ec) GTEST_SKIP() << "Symlinks unavailable: " << ec.message();
+ std::string error;
+ ASSERT_TRUE(TRestTools::MergeRootFilesTransactionally(alias.string(), {input.string()}, alias.string(),
+ true, &error))
+ << error;
+ EXPECT_TRUE(fs::is_symlink(alias));
+ auto file = TRestRootFileHandle::Open(target.string(), TRestRootFileMode::Read);
+ ASSERT_TRUE(file) << file.Error();
+ EXPECT_NE(file->Get<TNamed>("original"), nullptr);
+ EXPECT_NE(file->Get<TNamed>("incoming"), nullptr);
+}
11source/framework/test/src/RootIOConflict.cxxChanges since the earlier PR revisiontestsrisk: mediumimpact: high+67 / −0
- Intent
- Build incompatible layouts with the same class name/version in separate processes; demonstrate rejection when ROOT's cache resolves a different checksum.
- Invariant
- UPDATE/merge must fail before losing the original schema or replacing/removing files.
- Human review
- Verify both dictionaries deliberately use the same version and that assertions include source/output bytes. These fixtures ensure checksum conflicts cannot silently overwrite historical schemas.
diff --git a/source/framework/test/src/RootIOConflict.cxx b/source/framework/test/src/RootIOConflict.cxx new file mode 100644 index 00000000..e97de8ef --- /dev/null +++ b/source/framework/test/src/RootIOConflict.cxx
Hunk 1@@ -0,0 +1,67 @@
@@ -0,0 +1,67 @@
+#include <TFile.h>
+#include <TNamed.h>
+#include <TUUID.h>
+#include <gtest/gtest.h>
+
+#include <cstdlib>
+#include <filesystem>
+#include <fstream>
+#include <iterator>
+#include <string>
+#include <vector>
+
+namespace {
+std::vector<char> Bytes(const std::filesystem::path& path) {
+ std::ifstream input(path, std::ios::binary);
+ return {std::istreambuf_iterator<char>(input), std::istreambuf_iterator<char>()};
+}
+
+class RootIOConflict : public ::testing::Test {
+ protected:
+ std::filesystem::path directory;
+
+ void SetUp() override {
+ directory =
+ std::filesystem::temp_directory_path() / (std::string("rest-conflict-") + TUUID().AsString());
+ ASSERT_TRUE(std::filesystem::create_directory(directory));
+ }
+
+ void TearDown() override { std::filesystem::remove_all(directory); }
+
+ int Run(const char* executable, const std::vector<std::filesystem::path>& arguments) {
+ auto quote = [](const std::string& value) {
+ std::string result = "'";
+ for (const char c : value) result += c == '\'' ? "'\\''" : std::string(1, c);
+ return result + "'";
+ };
+ std::string command = quote(executable);
+ for (const auto& argument : arguments) command += " " + quote(argument.string());
+ return std::system(command.c_str());
+ }
+};
+} // namespace
+
+TEST_F(RootIOConflict, RejectUpdateWithoutChangingHistoricalSchemaOrBytes) {
+ const auto file = directory / "old.root";
+ ASSERT_EQ(Run(REST_IO_CONFLICT_Old, {file}), 0);
+ const auto before = Bytes(file);
+ ASSERT_FALSE(before.empty());
+ EXPECT_EQ(Run(REST_IO_CONFLICT_New, {file}), 0);
+ EXPECT_EQ(Bytes(file), before);
+}
+
+TEST_F(RootIOConflict, RejectWorkerSchemaConflictWithoutReplacingTargetOrRemovingInput) {
+ const auto source = directory / "old.root";
+ const auto target = directory / "target.root";
+ ASSERT_EQ(Run(REST_IO_CONFLICT_Old, {source}), 0);
+ {
+ TFile output(target.c_str(), "CREATE");
+ TNamed marker("marker", "original target");
+ marker.Write();
+ }
+ const auto sourceBefore = Bytes(source);
+ const auto targetBefore = Bytes(target);
+ EXPECT_EQ(Run(REST_IO_CONFLICT_New, {source, target}), 0);
+ EXPECT_EQ(Bytes(source), sourceBefore);
+ EXPECT_EQ(Bytes(target), targetBefore);
+}
12source/framework/tools/src/TRestTools.cxxChanges since the earlier PR revisionROOT corerisk: highimpact: critical+262 / −445
- Intent
- Centralize schema snapshots, rule registration, checked writable opens and transactional merges. Inventory and resolution share one pass; exception unwinding handles internal cleanup.
- Invariant
- Preserve exact historical user schema identities and rules; reject incompatible resolution before writes; validate both candidate and installed merge results.
- Human review
- Audit BuildCheck ownership, the narrow STL normalization exception, filesystem identity checks, and every failure/rollback boundary. This is the main semantic review.
diff --git a/source/framework/tools/src/TRestTools.cxx b/source/framework/tools/src/TRestTools.cxx index d7cbefa1..96196f52 100644 --- a/source/framework/tools/src/TRestTools.cxx +++ b/source/framework/tools/src/TRestTools.cxx
Hunk 1@@ -52,6 +52,7 @@
@@ -52,6 +52,7 @@ #include <TKey.h> #include <TList.h> #include <TObjString.h> +#include <TROOT.h> #include <TSchemaRule.h> #include <TSchemaRuleSet.h> #include <TStreamerInfo.h>
Hunk 2@@ -88,6 +89,7 @@
@@ -88,6 +89,7 @@ #include <limits> #include <memory> #include <optional> +#include <stdexcept> #include <thread> #include <tuple>
Hunk 3@@ -115,8 +117,13 @@ struct StreamerInfoIdentity {
@@ -115,8 +117,13 @@ struct StreamerInfoIdentity {
struct RootSchemaSnapshot {
std::set<StreamerInfoIdentity> streamerInfos;
std::set<std::string> schemaRules;
+ std::set<Int_t> numbers;
};
+bool IsImplementationSchema(const std::string& name) {
+ return TClassEdit::IsStdClass(name.c_str()) || TClassEdit::IsStdPairBase(name.c_str());
+}
+
struct RootContentEntry {
std::string className;
std::optional<Long64_t> treeEntries;
Hunk 4@@ -144,7 +151,9 @@ struct LocalFileIdentity {
@@ -144,7 +151,9 @@ struct LocalFileIdentity {
std::optional<std::filesystem::path> LocalRootPath(const std::string& filename) {
TUrl url(filename.c_str(), kTRUE);
if (!url.IsValid() || std::string(url.GetProtocol()) != "file") return std::nullopt;
- return std::filesystem::path(url.GetFile());
+ TString expanded(url.GetFile());
+ if (gSystem->ExpandPathName(expanded)) return std::nullopt;
+ return std::filesystem::path(expanded.Data());
}
std::optional<LocalFileIdentity> CaptureLocalFileIdentity(const std::filesystem::path& path,
Hunk 5@@ -173,16 +182,15 @@ std::optional<LocalFileIdentity> CaptureLocalFileIdentity(const std::filesystem:
@@ -173,16 +182,15 @@ std::optional<LocalFileIdentity> CaptureLocalFileIdentity(const std::filesystem:
return identity;
}
-std::string NormalizeSchemaRule(const std::string& rule, std::string* targetClass = nullptr) {
+std::string NormalizeSchemaRule(const std::string& rule) {
ROOT::TSchemaRule parsed;
if (!parsed.SetFromRule(rule.c_str())) return "";
TString normalized;
parsed.AsString(normalized);
- if (targetClass != nullptr) *targetClass = parsed.GetTargetClass();
return normalized.Data();
}
-bool ReadSchemaSnapshot(TFile& file, RootSchemaSnapshot& snapshot, std::string& error) {
+bool ReadSchemaSnapshot(TFile& file, RootSchemaSnapshot& snapshot, std::string& error, bool resolve = false) {
if (!file.IsOpen() || file.IsZombie()) {
error = "Cannot inspect StreamerInfos in a closed or invalid ROOT file";
return false;
Hunk 6@@ -217,7 +225,48 @@ bool ReadSchemaSnapshot(TFile& file, RootSchemaSnapshot& snapshot, std::string&
@@ -217,7 +225,48 @@ bool ReadSchemaSnapshot(TFile& file, RootSchemaSnapshot& snapshot, std::string&
snapshot.schemaRules.insert(normalized);
}
}
- return true;
+ if (!resolve) return true;
+
+ // BuildCheck transfers surviving descriptors into ROOT's global registry.
+ // Aliases and normalized implementation descriptors instead get kCanDelete,
+ // exactly as in TFile::ReadStreamerInfo. Inventory before this mutation.
+ infos->SetOwner(kFALSE);
+ bool resolved = true;
+ next.Reset();
+ while (TObject* object = next()) {
+ auto* info = dynamic_cast<TStreamerInfo*>(object);
+ if (info == nullptr) {
+ object->SetBit(TObject::kCanDelete);
+ continue;
+ }
+ const StreamerInfoIdentity identity{info->GetName(), info->GetClassVersion(), info->GetCheckSum()};
+ info->BuildCheck(&file);
+ if (info->GetNumber() > 0) {
+ snapshot.numbers.insert(info->GetNumber());
+ const auto* registered =
+ dynamic_cast<TStreamerInfo*>(gROOT->GetListOfStreamerInfo()->At(info->GetNumber()));
+ if (!IsImplementationSchema(identity.name) &&
+ (!registered || identity.name != registered->GetName() ||
+ // Streamer writes the absolute value of ROOT's internal version
+ // (for example ROOT::TIOFeatures uses a negative version).
+ identity.version != std::abs(registered->GetClassVersion()) ||
+ identity.checksum != registered->GetCheckSum())) {
+ error = "Conflicting StreamerInfo for " + identity.name + " version " +
+ std::to_string(identity.version) + " checksum " + std::to_string(identity.checksum);
+ if (registered)
+ error += "; resolved to version " + std::to_string(registered->GetClassVersion()) +
+ " checksum " + std::to_string(registered->GetCheckSum());
+ resolved = false;
+ }
+ } else if (!info->TestBit(TObject::kCanDelete)) {
+ error = "Cannot preserve StreamerInfo " + std::string(info->GetName()) + " version " +
+ std::to_string(info->GetClassVersion()) + " checksum " +
+ std::to_string(info->GetCheckSum());
+ resolved = false;
+ }
+ }
+ infos->Clear();
+ return resolved;
}
bool IsSubset(const RootSchemaSnapshot& expected, const RootSchemaSnapshot& actual, std::string& error) {
Hunk 7@@ -227,8 +276,7 @@ bool IsSubset(const RootSchemaSnapshot& expected, const RootSchemaSnapshot& actu
@@ -227,8 +276,7 @@ bool IsSubset(const RootSchemaSnapshot& expected, const RootSchemaSnapshot& actu
// standard-library types when it rewrites StreamerInfo. These are
// not user schemas; exact historical identities remain mandatory
// for every other class.
- if (TClassEdit::IsStdClass(info.name.c_str()) || TClassEdit::IsStdPairBase(info.name.c_str()))
- continue;
+ if (IsImplementationSchema(info.name)) continue;
error = "Merged file lost StreamerInfo " + info.name + " version " +
std::to_string(info.version) + " checksum " + std::to_string(info.checksum);
return false;
Hunk 8@@ -351,138 +399,64 @@ bool ValidateContentManifest(const RootContentManifest& expected, const RootCont
@@ -351,138 +399,64 @@ bool ValidateContentManifest(const RootContentManifest& expected, const RootCont
}
bool LoadOnDiskSchemaRules(const RootSchemaSnapshot& snapshot, std::string& error) {
- std::map<std::string, std::set<std::string>> rulesByTarget;
- for (const auto& rule : snapshot.schemaRules) {
- std::string targetClass;
- const std::string normalized = NormalizeSchemaRule(rule, &targetClass);
- if (normalized.empty() || targetClass.empty()) {
+ for (const auto& text : snapshot.schemaRules) {
+ auto rule = std::make_unique<ROOT::TSchemaRule>();
+ if (!rule->SetFromRule(text.c_str())) {
error = "Cannot parse an on-disk schema-evolution rule";
return false;
}
- rulesByTarget[targetClass].insert(normalized);
- }
-
- for (const auto& [targetClass, expectedRules] : rulesByTarget) {
- TClass* cl = TClass::GetClass(targetClass.c_str(), kFALSE);
- if (cl == nullptr) {
- error = "Cannot register schema rules for unloaded class " + targetClass;
+ const std::string target = rule->GetTargetClass();
+ TClass* cl = TClass::GetClass(target.c_str(), kFALSE);
+ if (!cl) {
+ error = "Cannot register schema rules for unloaded class " + target;
return false;
}
-
- auto registeredRules = [&cl]() {
- std::set<std::string> result;
- if (cl->GetSchemaRules() == nullptr) return result;
- TObjArrayIter nextRule(cl->GetSchemaRules()->GetRules());
- while (auto* registered = dynamic_cast<ROOT::TSchemaRule*>(nextRule())) {
+ auto registered = [&]() {
+ if (!cl->GetSchemaRules()) return false;
+ TIter next(cl->GetSchemaRules()->GetRules());
+ while (auto* candidate = dynamic_cast<ROOT::TSchemaRule*>(next())) {
TString serialized;
- registered->AsString(serialized);
- result.insert(serialized.Data());
+ candidate->AsString(serialized);
+ if (text == serialized.Data()) return true;
}
- return result;
+ return false;
};
-
- std::set<std::string> actualRules = registeredRules();
- for (const auto& expected : expectedRules) {
- if (actualRules.count(expected) != 0) continue;
-
- bool added = false;
- if (cl->TestBit(TClass::kIsEmulation)) {
- auto parsed = std::make_unique<ROOT::TSchemaRule>();
- if (parsed->SetFromRule(expected.c_str())) {
- auto* rules = cl->GetSchemaRules(kTRUE);
- added = rules != nullptr &&
- rules->AddRule(parsed.get(), ROOT::Detail::TSchemaRuleSet::kNoCheck);
- if (added) parsed.release();
- }
- } else {
- added = TClass::AddRule(expected.c_str());
- }
- if (!added) {
- error = "Cannot register an on-disk schema rule for class " + targetClass;
- return false;
- }
- actualRules = registeredRules();
- }
- for (const auto& expected : expectedRules) {
- if (actualRules.count(expected) == 0) {
- error = "Cannot preserve an on-disk schema rule for class " + targetClass;
- return false;
- }
- }
- }
- return true;
-}
-
-bool ResolveOnDiskStreamerInfos(TFile& file, std::vector<Int_t>& numbers, Int_t& maximumNumber,
- std::string& error) {
- numbers.clear();
- maximumNumber = 0;
- if (file.GetSeekInfo() == 0) return true;
-
- std::unique_ptr<TList> infos(file.GetStreamerInfoList());
- if (infos == nullptr) {
- error = "ROOT could not resolve the existing StreamerInfo record";
- return false;
- }
-
- // This mirrors TFile::ReadStreamerInfo. BuildCheck may transfer an
- // on-disk TStreamerInfo into ROOT's global registry, while aliases and
- // ROOT-managed implementation descriptors are marked kCanDelete. An
- // owning TList would destroy the globally registered objects.
- infos->SetOwner(kFALSE);
- std::set<Int_t> resolvedNumbers;
- std::string unresolvedError;
- TIter next(infos.get());
- while (TObject* object = next()) {
- auto* info = dynamic_cast<TStreamerInfo*>(object);
- if (info == nullptr) {
- object->SetBit(TObject::kCanDelete);
- continue;
+ if (registered()) continue;
+ bool added;
+ if (cl->TestBit(TClass::kIsEmulation)) {
+ auto* rules = cl->GetSchemaRules(kTRUE);
+ added = rules && rules->AddRule(rule.get(), ROOT::Detail::TSchemaRuleSet::kNoCheck);
+ if (added) rule.release();
+ } else {
+ added = TClass::AddRule(text.c_str());
}
-
- info->BuildCheck(&file);
- const Int_t number = info->GetNumber();
- if (number > 0) {
- resolvedNumbers.insert(number);
- maximumNumber = std::max(maximumNumber, number);
- } else if (!info->TestBit(TObject::kCanDelete) && unresolvedError.empty()) {
- unresolvedError = "Cannot preserve StreamerInfo " + std::string(info->GetName()) + " version " +
- std::to_string(info->GetClassVersion()) + " checksum " +
- std::to_string(info->GetCheckSum());
+ if (!added || !registered()) {
+ error = "Cannot preserve an on-disk schema rule for class " + target;
+ return false;
}
}
- infos->Clear();
-
- if (!unresolvedError.empty()) {
- error = unresolvedError;
- return false;
- }
- numbers.assign(resolvedNumbers.begin(), resolvedNumbers.end());
- return true;
-}
-
-bool ResolveSchemaUpdate(TFile& file, std::vector<Int_t>& numbers, Int_t& maximumNumber, std::string& error) {
- RootSchemaSnapshot snapshot;
- if (!ReadSchemaSnapshot(file, snapshot, error) ||
- !ResolveOnDiskStreamerInfos(file, numbers, maximumNumber, error) ||
- !LoadOnDiskSchemaRules(snapshot, error))
- return false;
return true;
}
-bool ApplySchemaUpdate(TFile& file, const std::vector<Int_t>& numbers, Int_t maximumNumber,
- std::string& error) {
+bool ApplySchemaUpdate(TFile& file, const std::set<Int_t>& numbers, std::string& error) {
TArrayC* classIndex = file.GetClassIndex();
if (classIndex == nullptr) {
error = "ROOT file has no StreamerInfo class index";
return false;
}
- if (maximumNumber >= classIndex->GetSize()) classIndex->Set(maximumNumber + 1);
+ if (!numbers.empty() && *numbers.rbegin() >= classIndex->GetSize())
+ classIndex->Set(*numbers.rbegin() + 1);
for (const Int_t number : numbers) classIndex->fArray[number] = 1;
classIndex->fArray[0] = 1;
return true;
}
+// Internal operations throw to unwind file owners before cleaning up a failed
+// transaction. The public API translates failures back to bool plus Error().
+void Require(bool success, const std::string& error) {
+ if (!success) throw std::runtime_error(error);
+}
+
std::filesystem::path UniqueSiblingPath(const std::filesystem::path& target, const std::string& label) {
for (int attempt = 0; attempt < 20; ++attempt) {
TString uuid = TUUID().AsString();
Hunk 9@@ -568,24 +542,41 @@ TRestRootFileHandle TRestRootFileHandle::Open(const std::string& filename, TRest
@@ -568,24 +542,41 @@ TRestRootFileHandle TRestRootFileHandle::Open(const std::string& filename, TRest
return result;
}
- const char* option = mode == TRestRootFileMode::Read
- ? "READ"
- : (mode == TRestRootFileMode::Recreate ? "RECREATE" : "READ");
+ const char* option = mode == TRestRootFileMode::Recreate ? "RECREATE" : "READ";
+ bool createForUpdate = false;
+ if (mode == TRestRootFileMode::Update) {
+ std::error_code ec;
+ const auto path = LocalRootPath(filename);
+ if (!path) {
+ result.fError = "Cannot resolve local UPDATE destination '" + filename + "'";
+ return result;
+ }
+ createForUpdate = !std::filesystem::exists(*path, ec);
+ if (ec) {
+ result.fError = "Cannot inspect ROOT destination '" + filename + "': " + ec.message();
+ return result;
+ }
+ // ROOT UPDATE creates missing files. CREATE preserves that contract
+ // without overwriting a file appearing between this check and Open.
+ if (createForUpdate) option = "CREATE";
+ }
result.fFile.reset(TFile::Open(filename.c_str(), option));
if (result.fFile == nullptr || !result.fFile->IsOpen() || result.fFile->IsZombie()) {
- result.fError = "Cannot open ROOT file '" + filename +
- (mode == TRestRootFileMode::Update ? "' for read-only UPDATE preflight"
- : "' in mode " + std::string(option));
+ result.fError =
+ "Cannot open ROOT file '" + filename +
+ (mode == TRestRootFileMode::Update && !createForUpdate ? "' for read-only UPDATE preflight"
+ : "' in mode " + std::string(option));
result.fFile.reset();
return result;
}
- if (mode == TRestRootFileMode::Recreate && !result.fFile->IsWritable()) {
+ if ((mode == TRestRootFileMode::Recreate || createForUpdate) && !result.fFile->IsWritable()) {
result.fError = "ROOT file '" + filename + "' is not writable";
result.fFile.reset();
return result;
}
- if (mode == TRestRootFileMode::Update && !PrepareBorrowedUpdate(*result.fFile, &result.fError)) {
+ if (mode == TRestRootFileMode::Update && !createForUpdate &&
+ !PrepareBorrowedUpdate(*result.fFile, &result.fError)) {
result.fFile.reset();
}
return result;
Hunk 10@@ -615,71 +606,39 @@ bool TRestRootFileHandle::Close() noexcept {
@@ -615,71 +606,39 @@ bool TRestRootFileHandle::Close() noexcept {
}
bool TRestRootFileHandle::PrepareBorrowedUpdate(TFile& file, std::string* error) {
- std::string localError;
- if (!file.IsOpen() || file.IsZombie() || file.IsWritable()) {
- localError = "UPDATE preparation requires a valid TFile that is currently open in READ mode";
- if (error != nullptr) *error = localError;
- return false;
- }
-
- const std::string filename = file.GetName();
- if (TRestTools::IsRemoteRootPath(filename)) {
- localError = "REST refuses to mutate remote ROOT destination '" + filename +
- "'. Choose an explicit local output file.";
- if (error != nullptr) *error = localError;
- return false;
- }
-
- std::vector<Int_t> numbers;
- Int_t maximumNumber = 0;
- if (!ResolveSchemaUpdate(file, numbers, maximumNumber, localError)) {
- if (error != nullptr) *error = localError;
- return false;
- }
-
- const auto localPath = LocalRootPath(filename);
- if (!localPath) {
- localError = "Cannot resolve local ROOT path '" + filename + "'";
- if (error != nullptr) *error = localError;
- return false;
- }
- const std::string uuidBefore = file.GetUUID().AsString();
- const auto identityBefore = CaptureLocalFileIdentity(*localPath, localError);
- if (!identityBefore) {
- if (error != nullptr) *error = localError;
- return false;
- }
-
+ bool transitionAttempted = false;
+ try {
+ Require(file.IsOpen() && !file.IsZombie() && !file.IsWritable(),
+ "UPDATE preparation requires a valid TFile that is currently open in READ mode");
+ const std::string filename = file.GetName();
+ const auto path = LocalRootPath(filename);
+ Require(path.has_value(), "REST refuses to mutate remote ROOT destination '" + filename +
+ "'. Choose an explicit local output file.");
+ std::string detail;
+ const auto before = CaptureLocalFileIdentity(*path, detail);
+ Require(before.has_value(), detail);
+ RootSchemaSnapshot schema;
+ Require(ReadSchemaSnapshot(file, schema, detail, true) && LoadOnDiskSchemaRules(schema, detail),
+ detail);
#ifdef REST_TESTING_ENABLED
- if (gForceNextRootUpdatePreflightFailure.exchange(false)) {
- localError = "Forced writable ROOT preflight failure for testing";
- if (error != nullptr) *error = localError;
- return false;
- }
+ Require(!gForceNextRootUpdatePreflightFailure.exchange(false),
+ "Forced writable ROOT preflight failure for testing");
#endif
-
- if (file.ReOpen("UPDATE") != 0 || !file.IsWritable()) {
- localError = "ROOT could not transition '" + filename + "' from the validated READ handle to UPDATE";
- if (file.IsWritable()) file.SetWritable(kFALSE);
- if (error != nullptr) *error = localError;
- return false;
- }
-
- const auto identityAfter = CaptureLocalFileIdentity(*localPath, localError);
- const std::string uuidAfter = file.GetUUID().AsString();
- if (!identityAfter || !(*identityBefore == *identityAfter) || uuidBefore != uuidAfter) {
- if (localError.empty())
- localError = "ROOT file '" + filename + "' changed during the READ-to-UPDATE transition";
- file.SetWritable(kFALSE);
- if (error != nullptr) *error = localError;
- return false;
- }
- if (!ApplySchemaUpdate(file, numbers, maximumNumber, localError)) {
- file.SetWritable(kFALSE);
- if (error != nullptr) *error = localError;
+ transitionAttempted = true;
+ Require(file.ReOpen("UPDATE") == 0 && file.IsWritable(),
+ "ROOT could not transition '" + filename + "' from validated READ to UPDATE");
+ const auto after = CaptureLocalFileIdentity(*path, detail);
+ Require(after.has_value(), detail);
+ Require(*before == *after,
+ "ROOT file '" + filename + "' changed during the READ-to-UPDATE transition");
+ Require(ApplySchemaUpdate(file, schema.numbers, detail), detail);
+ if (error) error->clear();
+ return true;
+ } catch (const std::exception& failure) {
+ if (transitionAttempted) file.SetWritable(kFALSE);
+ if (error) *error = failure.what();
return false;
}
- return true;
}
///////////////////////////////////////////////
Hunk 11@@ -1410,282 +1369,140 @@ bool TRestTools::MergeRootFilesTransactionally(const std::string& outputFile,
@@ -1410,282 +1369,140 @@ bool TRestTools::MergeRootFilesTransactionally(const std::string& outputFile,
const std::vector<std::string>& inputFiles,
const std::string& existingTarget, bool removeInputsOnSuccess,
std::string* error) {
- std::string localError;
- if (IsRemoteRootPath(outputFile)) {
- localError = "REST refuses to replace remote ROOT destination '" + outputFile +
- "'. Choose an explicit local output file.";
- if (error != nullptr) *error = localError;
- return false;
- }
-
- const auto localOutput = LocalRootPath(outputFile);
- if (!localOutput) {
- localError = "Cannot resolve local ROOT output path '" + outputFile + "'";
- if (error != nullptr) *error = localError;
- return false;
- }
- std::error_code ec;
- const std::filesystem::path target = std::filesystem::absolute(*localOutput, ec);
- if (ec) {
- localError = "Cannot resolve absolute output path '" + outputFile + "': " + ec.message();
- if (error != nullptr) *error = localError;
- return false;
- }
- if (target.parent_path().empty() || !std::filesystem::exists(target.parent_path())) {
- localError = "Output directory does not exist for '" + target.string() + "'";
- if (error != nullptr) *error = localError;
- return false;
- }
-
- std::vector<std::string> sources;
- if (!existingTarget.empty()) sources.push_back(existingTarget);
- sources.insert(sources.end(), inputFiles.begin(), inputFiles.end());
- if (sources.empty()) {
- localError = "No ROOT input files were provided for transactional merge";
- if (error != nullptr) *error = localError;
- return false;
- }
-
- RootSchemaSnapshot expectedSchema;
- RootContentManifest expectedContent;
- RootContentManifest expectedInputContent;
- std::set<Int_t> resolvedSchemaNumbers;
- Int_t maximumSchemaNumber = 0;
- for (std::size_t sourceIndex = 0; sourceIndex < sources.size(); ++sourceIndex) {
- const auto& sourceName = sources[sourceIndex];
- std::unique_ptr<TFile> source(TFile::Open(sourceName.c_str(), "READ"));
- RootSchemaSnapshot sourceSchema;
- RootContentManifest sourceContent;
- std::vector<Int_t> sourceSchemaNumbers;
- Int_t sourceMaximumNumber = 0;
- if (source == nullptr || !source->IsOpen() || source->IsZombie()) {
- localError = "Cannot open ROOT merge input '" + sourceName + "'";
- if (error != nullptr) *error = localError;
- return false;
- }
- if (!ReadSchemaSnapshot(*source, sourceSchema, localError) ||
- !ResolveOnDiskStreamerInfos(*source, sourceSchemaNumbers, sourceMaximumNumber, localError) ||
- !LoadOnDiskSchemaRules(sourceSchema, localError) ||
- !ReadContentManifest(*source, "", sourceContent, localError)) {
- localError = "Cannot inspect ROOT merge input '" + sourceName + "': " + localError;
- if (error != nullptr) *error = localError;
- return false;
+ namespace fs = std::filesystem;
+ fs::path temporary;
+ try {
+ const auto localOutput = LocalRootPath(outputFile);
+ Require(localOutput.has_value(), "REST refuses to replace remote ROOT destination '" + outputFile +
+ "'. Choose an explicit local output file.");
+ const fs::path target = fs::weakly_canonical(fs::absolute(*localOutput));
+ Require(fs::is_directory(target.parent_path()),
+ "Output directory does not exist for '" + target.string() + "'");
+
+ std::vector<std::string> sources;
+ if (!existingTarget.empty()) sources.push_back(existingTarget);
+ sources.insert(sources.end(), inputFiles.begin(), inputFiles.end());
+ Require(!sources.empty(), "No ROOT input files were provided for transactional merge");
+
+ RootSchemaSnapshot schema;
+ RootContentManifest content, workers;
+ std::string detail;
+ for (std::size_t index = 0; index < sources.size(); ++index) {
+ auto source = TRestRootFileHandle::Open(sources[index], TRestRootFileMode::Read);
+ Require(bool(source), source.Error());
+ RootContentManifest incoming;
+ if (!ReadSchemaSnapshot(*source, schema, detail, true) ||
+ !LoadOnDiskSchemaRules(schema, detail) ||
+ !ReadContentManifest(*source, "", incoming, detail) ||
+ !AddContentManifest(incoming, !existingTarget.empty() && index == 0 ? content : workers,
+ detail))
+ throw std::runtime_error("Cannot prepare merge input '" + sources[index] + "': " + detail);
}
- RootContentManifest& accumulatedContent =
- !existingTarget.empty() && sourceIndex == 0 ? expectedContent : expectedInputContent;
- if (!AddContentManifest(sourceContent, accumulatedContent, localError)) {
- localError = "Cannot merge ROOT input '" + sourceName + "': " + localError;
- if (error != nullptr) *error = localError;
- return false;
- }
- expectedSchema.streamerInfos.insert(sourceSchema.streamerInfos.begin(),
- sourceSchema.streamerInfos.end());
- expectedSchema.schemaRules.insert(sourceSchema.schemaRules.begin(), sourceSchema.schemaRules.end());
- resolvedSchemaNumbers.insert(sourceSchemaNumbers.begin(), sourceSchemaNumbers.end());
- maximumSchemaNumber = std::max(maximumSchemaNumber, sourceMaximumNumber);
- }
-
- // Match TFileMerger's historical UPDATE contract: worker inputs merge
- // with one another, then replace same-named target objects. Target-only
- // keys remain opaque. Treating the target as another additive input is
- // what caused #567 to rewrite existing TRestAnalysisTree objects.
- for (const auto& [path, inputEntry] : expectedInputContent) {
- const auto targetEntry = expectedContent.find(path);
- if (targetEntry != expectedContent.end() && targetEntry->second.className != inputEntry.className) {
- localError = "ROOT merge input key '" + path + "' has incompatible classes '" +
- targetEntry->second.className + "' and '" + inputEntry.className + "'";
- if (error != nullptr) *error = localError;
- return false;
+ // ROOT UPDATE merges workers together, replacing same-named target
+ // objects; target-only objects stay in the byte-for-byte seed untouched.
+ for (const auto& [path, incoming] : workers) {
+ const auto found = content.find(path);
+ Require(found == content.end() || found->second.className == incoming.className,
+ "ROOT merge input key '" + path + "' has incompatible classes");
+ content[path] = incoming;
}
- expectedContent[path] = inputEntry;
- }
-
- const std::filesystem::path temporary = UniqueSiblingPath(target, "merge");
- const std::filesystem::path backup = UniqueSiblingPath(target, "backup");
- if (temporary.empty() || backup.empty()) {
- localError = "Cannot allocate temporary paths next to '" + target.string() + "'";
- if (error != nullptr) *error = localError;
- return false;
- }
- auto removeTemporary = [&temporary](std::string& primaryError) {
- std::error_code cleanupError;
- std::filesystem::remove(temporary, cleanupError);
- if (cleanupError)
- primaryError += ". REST also could not remove temporary file '" + temporary.string() +
- "': " + cleanupError.message();
- };
-
- if (!existingTarget.empty()) {
- const auto localExisting = LocalRootPath(existingTarget);
- if (localExisting) {
- std::filesystem::copy_file(*localExisting, temporary, std::filesystem::copy_options::none, ec);
- if (ec)
- localError =
- "Cannot seed temporary merge output from '" + existingTarget + "': " + ec.message();
- } else if (!TFile::Cp(existingTarget.c_str(), temporary.c_str(), kFALSE)) {
- localError = "ROOT could not seed temporary merge output from '" + existingTarget + "'";
+ temporary = UniqueSiblingPath(target, "merge");
+ const auto backup = UniqueSiblingPath(target, "backup");
+ Require(!temporary.empty() && !backup.empty(),
+ "Cannot allocate temporary paths next to '" + target.string() + "'");
+ if (!existingTarget.empty()) {
+ const auto localExisting = LocalRootPath(existingTarget);
+ if (localExisting)
+ fs::copy_file(*localExisting, temporary);
+ else
+ Require(TFile::Cp(existingTarget.c_str(), temporary.c_str(), kFALSE),
+ "ROOT could not seed temporary merge output from '" + existingTarget + "'");
}
- }
-
- TRestRootFileHandle outputHandle;
- if (localError.empty()) {
- outputHandle =
- TRestRootFileHandle::Open(temporary.string(), existingTarget.empty() ? TRestRootFileMode::Recreate
- : TRestRootFileMode::Update);
- if (!outputHandle)
- localError =
- "Cannot prepare temporary merge output '" + temporary.string() + "': " + outputHandle.Error();
- }
- const std::vector<Int_t> allSchemaNumbers(resolvedSchemaNumbers.begin(), resolvedSchemaNumbers.end());
- if (localError.empty() &&
- !ApplySchemaUpdate(*outputHandle, allSchemaNumbers, maximumSchemaNumber, localError)) {
- localError = "Cannot preserve input schemas in temporary merge output '" + temporary.string() +
- "': " + localError;
- }
-
- if (localError.empty() && !inputFiles.empty()) {
- TFileMerger merger(kFALSE);
- merger.SetPrintLevel(0);
- if (!merger.OutputFile(std::move(outputHandle.fFile))) {
- localError = "ROOT could not adopt temporary merge output '" + temporary.string() + "'";
- } else {
- // An existing destination seeds the temporary file byte-for-byte.
- // Only new worker inputs are merged, preserving the historical
- // TFileMerger UPDATE semantics for target-only objects.
- for (const auto& source : inputFiles) {
- if (!merger.AddFile(source.c_str(), kFALSE)) {
- localError = "ROOT could not add merge input '" + source + "'";
- break;
- }
+ {
+ auto output = TRestRootFileHandle::Open(temporary.string(), existingTarget.empty()
+ ? TRestRootFileMode::Recreate
+ : TRestRootFileMode::Update);
+ Require(bool(output), output.Error());
+ Require(ApplySchemaUpdate(*output, schema.numbers, detail), detail);
+ if (inputFiles.empty()) {
+ Require(output.Close(), output.Error());
+ } else {
+ TFileMerger merger(kFALSE);
+ merger.SetPrintLevel(0);
+ Require(merger.OutputFile(std::move(output.fFile)),
+ "ROOT could not adopt temporary merge output '" + temporary.string() + "'");
+ for (const auto& input : inputFiles)
+ Require(merger.AddFile(input.c_str(), kFALSE),
+ "ROOT could not add merge input '" + input + "'");
+ Require(merger.Merge(), "ROOT failed while merging into '" + temporary.string() + "'");
}
}
- if (localError.empty() && !merger.Merge())
- localError = "ROOT failed while merging into temporary output '" + temporary.string() + "'";
- } else if (localError.empty() && !outputHandle.Close()) {
- localError =
- "ROOT reported a write error while preparing temporary output '" + temporary.string() + "'";
- }
- if (!localError.empty()) {
- removeTemporary(localError);
- if (error != nullptr) *error = localError;
- return false;
- }
-
- bool temporaryValid = false;
- {
- std::unique_ptr<TFile> merged(TFile::Open(temporary.c_str(), "READ"));
- RootSchemaSnapshot actualSchema;
- RootContentManifest actualContent;
- temporaryValid = merged != nullptr && merged->IsOpen() && !merged->IsZombie() &&
- ReadSchemaSnapshot(*merged, actualSchema, localError) &&
- IsSubset(expectedSchema, actualSchema, localError) &&
- ReadContentManifest(*merged, "", actualContent, localError) &&
- ValidateContentManifest(expectedContent, actualContent, localError);
- }
- if (!temporaryValid) {
- if (localError.empty()) localError = "Cannot validate temporary ROOT merge output";
- removeTemporary(localError);
- if (error != nullptr) *error = localError;
- return false;
- }
-
- const bool targetExisted = std::filesystem::exists(target);
- if (targetExisted) {
- const auto permissions = std::filesystem::status(target, ec).permissions();
- if (ec) {
- localError = "Cannot inspect permissions of '" + target.string() + "': " + ec.message();
- removeTemporary(localError);
- if (error != nullptr) *error = localError;
- return false;
- }
- std::filesystem::permissions(temporary, permissions, ec);
- if (ec) {
- localError = "Cannot preserve permissions on temporary ROOT output: " + ec.message();
- removeTemporary(localError);
- if (error != nullptr) *error = localError;
- return false;
- }
- }
-
- if (!ReplaceLocalFile(temporary, target, backup, localError)) {
- removeTemporary(localError);
- if (error != nullptr) *error = localError;
- return false;
- }
- bool replacementValid = false;
- {
- std::unique_ptr<TFile> merged(TFile::Open(target.c_str(), "READ"));
- RootSchemaSnapshot actualSchema;
- RootContentManifest actualContent;
- replacementValid = merged != nullptr && merged->IsOpen() && !merged->IsZombie() &&
- ReadSchemaSnapshot(*merged, actualSchema, localError) &&
- IsSubset(expectedSchema, actualSchema, localError) &&
- ReadContentManifest(*merged, "", actualContent, localError) &&
- ValidateContentManifest(expectedContent, actualContent, localError);
- if (!replacementValid && localError.empty())
- localError = "Cannot validate replaced ROOT output '" + target.string() + "'";
- }
+ auto validate = [&](const fs::path& path) {
+ auto file = TRestRootFileHandle::Open(path.string(), TRestRootFileMode::Read);
+ Require(bool(file), file.Error());
+ RootSchemaSnapshot actualSchema;
+ RootContentManifest actualContent;
+ Require(ReadSchemaSnapshot(*file, actualSchema, detail) &&
+ IsSubset(schema, actualSchema, detail) &&
+ ReadContentManifest(*file, "", actualContent, detail) &&
+ ValidateContentManifest(content, actualContent, detail),
+ detail);
+ };
+ validate(temporary);
+ const bool targetExisted = fs::exists(target);
+ if (targetExisted) fs::permissions(temporary, fs::status(target).permissions());
+ Require(ReplaceLocalFile(temporary, target, backup, detail), detail);
+ try {
+ validate(target);
#ifdef REST_TESTING_ENABLED
- if (gForceNextMergeValidationFailure.exchange(false)) {
- replacementValid = false;
- localError = "Forced post-replacement ROOT validation failure for testing";
- }
+ Require(!gForceNextMergeValidationFailure.exchange(false),
+ "Forced post-replacement ROOT validation failure for testing");
#endif
- if (!replacementValid) {
- std::string rollbackError;
- if (!RollBackLocalFile(target, backup, targetExisted, rollbackError))
- localError +=
- ". Rollback also failed: " + rollbackError + ". The backup is at '" + backup.string() + "'";
- if (error != nullptr) *error = localError;
- return false;
- }
-
- if (targetExisted) {
- ec.clear();
- std::filesystem::remove(backup, ec);
- if (ec) {
- localError = "Merged output is valid, but REST could not remove rollback backup '" +
- backup.string() + "': " + ec.message();
- if (error != nullptr) *error = localError;
- return false;
+ } catch (const std::exception& failure) {
+ std::string rollbackError;
+ detail = failure.what();
+ if (!RollBackLocalFile(target, backup, targetExisted, rollbackError))
+ detail += ". Rollback also failed: " + rollbackError + ". The backup is at '" +
+ backup.string() + "'";
+ throw std::runtime_error(detail);
}
- }
- if (removeInputsOnSuccess) {
- std::vector<std::string> removalFailures;
- for (const auto& input : inputFiles) {
- const auto localInput = LocalRootPath(input);
- if (!localInput) continue;
- ec.clear();
- const auto absoluteInput = std::filesystem::absolute(*localInput, ec);
- if (ec) {
- removalFailures.push_back("'" + localInput->string() +
- "': cannot resolve absolute path: " + ec.message());
- continue;
- }
- if (absoluteInput == target) continue;
- const bool removed = std::filesystem::remove(absoluteInput, ec);
- std::error_code existsError;
- const bool stillExists = std::filesystem::exists(absoluteInput, existsError);
- if (ec || existsError || (!removed && stillExists))
- removalFailures.push_back(
- "'" + absoluteInput.string() + "': " +
- (ec ? ec.message() : (existsError ? existsError.message() : "file was not removed")));
+ if (targetExisted) {
+ std::error_code ec;
+ fs::remove(backup, ec);
+ Require(!ec, "Merged output is valid, but REST could not remove rollback backup '" +
+ backup.string() + "': " + ec.message());
}
- if (!removalFailures.empty()) {
- localError = "Merged output is valid, but REST could not remove input file(s): ";
- for (std::size_t index = 0; index < removalFailures.size(); ++index) {
- if (index != 0) localError += "; ";
- localError += removalFailures[index];
+ if (removeInputsOnSuccess) {
+ std::string failures;
+ for (const auto& input : inputFiles) {
+ const auto localInput = LocalRootPath(input);
+ if (!localInput) continue;
+ std::error_code ec;
+ const auto absoluteInput = fs::weakly_canonical(fs::absolute(*localInput), ec);
+ if (!ec && absoluteInput == target) continue;
+ if (!ec) fs::remove(*localInput, ec);
+ if (ec) failures += "'" + input + "': " + ec.message() + "; ";
}
- if (error != nullptr) *error = localError;
- return false;
+ Require(failures.empty(),
+ "Merged output is valid, but REST could not remove input file(s): " + failures);
+ }
+ if (error) error->clear();
+ return true;
+ } catch (const std::exception& failure) {
+ // All ROOT owners have unwound before unlinking the candidate, including
+ // failures in TFileMerger adoption, schema registration or filesystem APIs.
+ std::string detail = failure.what();
+ if (!temporary.empty()) {
+ std::error_code ec;
+ std::filesystem::remove(temporary, ec);
+ if (ec) detail += ". Cannot remove temporary file '" + temporary.string() + "': " + ec.message();
}
+ if (error) *error = detail;
+ return false;
}
- if (error != nullptr) error->clear();
- return true;
}
///////////////////////////////////////////////