From fd953ab28053cb737390fb18f156aa0ed2693a65 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Wed, 10 Jun 2026 14:56:10 -0400 Subject: [PATCH 1/3] fix: RNTuple footer leak on extend, broken WritableNTuple.compression, directory __delitem__ KeyError - _cascadentuple.py extend(): release full key+payload region (num_bytes+compressed_bytes) instead of just the key header (allocation/num_bytes), preventing footer-sized FreeSegments leak on every extend call - WritableNTuple.compression getter/setter: replace copied-from-WritableTree code that referenced nonexistent _branch_data; getter now returns actual file-level compression; setter raises NotImplementedError with a clear message - WritableDirectory._del: check get_key() result for None and raise KeyInFileError, fixing AttributeError on nonexistent key deletion (MutableMapping contract) - Remove dead/broken NTuple code: Ntuple_PageLink class (unused, broken serialize), NTuple.branch_types and NTuple.field_name properties (referencing unset attributes), NTuple_Footer.__repr__ (referenced nonexistent metadata_block_envelope_links), NTuple.write_updates (stub never called) - Remove Tree.branch_types property in _cascadetree.py (referencing unset _branch_types) Assisted-by: ClaudeCode:claude-sonnet-4-6 --- src/uproot/writing/_cascadentuple.py | 31 ++---------- src/uproot/writing/_cascadetree.py | 4 -- src/uproot/writing/writable.py | 76 ++++++---------------------- 3 files changed, 20 insertions(+), 91 deletions(-) diff --git a/src/uproot/writing/_cascadentuple.py b/src/uproot/writing/_cascadentuple.py index d183d5db2..ebca2a72f 100644 --- a/src/uproot/writing/_cascadentuple.py +++ b/src/uproot/writing/_cascadentuple.py @@ -497,7 +497,7 @@ def __init__(self, location, header_checksum): super().__init__(location, None) def __repr__(self): - return f"{type(self).__name__}(extension_field_record_frames = {self.extension_field_record_frames}, column_group_record_frames = {self.extension_column_record_frames}, cluster_summary_record_frames={self.extension_column_record_frames}, cluster_group_record_frames{self.cluster_group_record_frames}, metadata_block_envelope_link = {self.metadata_block_envelope_links})" + return f"{type(self).__name__}(extension_field_record_frames={self.extension_field_record_frames}, extension_column_record_frames={self.extension_column_record_frames}, cluster_group_record_frames={self.cluster_group_record_frames})" def serialize(self): out = [] @@ -756,19 +756,6 @@ def serialize(self): return out -class Ntuple_PageLink: - def __init__(self, num_bytes, offset): - self.num_bytes = num_bytes - self.offset = offset - - def serialize(self): - outbytes = _rntuple_locator_size_format.pack(self.num_bytes, self.offset) - return outbytes - - def __repr__(self): - return f"{type(self).__name__}({self.num_bytes}, {self.offset})" - - class NTuple(CascadeNode): """ Writes a RNTuple, including all fields, columns, and (upon ``extend``) pages. @@ -827,18 +814,10 @@ def name(self): def title(self): return self._key.title - @property - def branch_types(self): - return self._branch_types - @property def freesegments(self): return self._freesegments - @property - def field_name(self): - return self._field_name - @property def location(self): return self._key.location @@ -956,7 +935,10 @@ def extend(self, file, sink, data): old_footer_key = self._footer_key self._freesegments.release( - old_footer_key.location, old_footer_key.location + old_footer_key.allocation + old_footer_key.location, + old_footer_key.location + + old_footer_key.num_bytes + + old_footer_key.compressed_bytes, ) footer_raw_data = self._footer.serialize() self._footer_key = self.add_rblob(sink, footer_raw_data, len(footer_raw_data)) @@ -1042,9 +1024,6 @@ def write(self, sink): self._freesegments.write(sink) - def write_updates(self, sink): - sink.flush() - def _to_packed_form(form): match form.__class__: diff --git a/src/uproot/writing/_cascadetree.py b/src/uproot/writing/_cascadetree.py index 6b80763d3..869b41e0c 100644 --- a/src/uproot/writing/_cascadetree.py +++ b/src/uproot/writing/_cascadetree.py @@ -414,10 +414,6 @@ def name(self): def title(self): return self._key.title - @property - def branch_types(self): - return self._branch_types - @property def freesegments(self): return self._freesegments diff --git a/src/uproot/writing/writable.py b/src/uproot/writing/writable.py index 7d45223f4..f60b1af6a 100644 --- a/src/uproot/writing/writable.py +++ b/src/uproot/writing/writable.py @@ -1056,6 +1056,14 @@ def get_chunk(start, stop): def _del(self, name, cycle): key = self._cascading.data.get_key(name, cycle) + if key is None: + raise uproot.exceptions.KeyInFileError( + name, + cycle="any" if cycle is None else cycle, + keys=self._cascading.data.key_names, + file_path=self.file_path, + object_path=self.object_path, + ) start = key.seek_location stop = start + key.num_bytes + key.compressed_bytes self._cascading.freesegments.release(start, stop) @@ -2127,72 +2135,18 @@ def __exit__(self, exception_type, exception_value, traceback): def compression(self): """ Compression algorithm and level (:doc:`uproot.compression.Compression` or None) - for new blobs added to the RNTuple. - - This property can be changed and doesn't have to be the same as the compression - of the file, which allows you to write different objects with different - compression settings. - - The following are equivalent: + used when writing pages to this RNTuple. - .. code-block:: python - - my_directory["tree"]["branch1"].compression = uproot.ZLIB(1) - my_directory["tree"]["branch2"].compression = uproot.LZMA(9) - - and - - .. code-block:: python - - my_directory["tree"].compression = {"branch1": uproot.ZLIB(1), - "branch2": uproot.LZMA(9)} + RNTuple compression is file-wide and is taken from the file header; individual + per-column compression is not yet supported. """ - out = {} - last = None - for datum in self._cascading._branch_data: - if datum["kind"] != "record": - last = out[datum["fName"]] = datum["compression"] - if all(x == last for x in out.values()): - return last - else: - return out + return self._cascading._freesegments.fileheader.compression @compression.setter def compression(self, value): - if value is None or isinstance(value, uproot.compression.Compression): - for datum in self._cascading._branch_data: - if datum["kind"] != "record": - datum["compression"] = value - - elif ( - isinstance(value, Mapping) - and all( - isinstance(k, str) - and (v is None or isinstance(v, uproot.compression.Compression)) - for k, v in value.items() - ) - and all( - datum["fName"] in value - for datum in self._cascading._branch_data - if datum["kind"] != "record" - ) - and len(value) - == len( - [ - datum - for datum in self._cascading._branch_data - if datum["kind"] != "record" - ] - ) - ): - for datum in self._cascading._branch_data: - if datum["kind"] != "record": - datum["compression"] = value[datum["fName"]] - - else: - raise TypeError( - "compression must be None, a uproot.compression.Compression object, like uproot.ZLIB(4) or uproot.ZSTD(0), or a mapping of branch names to such objects" - ) + raise NotImplementedError( + "per-RNTuple compression is not yet supported; set compression on the file instead" + ) @property def num_entries(self) -> int: From da0aef63010699eecdf9688fe64e53ff7f98c361 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Wed, 10 Jun 2026 14:57:17 -0400 Subject: [PATCH 2/3] test: regression tests for PR #1650 writing bugfixes Add tests/test_1650_rntuple_writing_bugfixes.py covering: - RNTuple extend() FreeSegments leak: verify file stays small and is readable after 10 extends - WritableNTuple.compression getter: returns file-level compression without AttributeError - WritableNTuple.compression setter: raises NotImplementedError as expected - WritableDirectory.__delitem__ on nonexistent key: raises KeyError (was AttributeError) - WritableDirectory.__delitem__ on nonexistent key: specifically raises KeyInFileError Assisted-by: ClaudeCode:claude-sonnet-4-6 --- tests/test_1650_rntuple_writing_bugfixes.py | 109 ++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 tests/test_1650_rntuple_writing_bugfixes.py diff --git a/tests/test_1650_rntuple_writing_bugfixes.py b/tests/test_1650_rntuple_writing_bugfixes.py new file mode 100644 index 000000000..2e2001a25 --- /dev/null +++ b/tests/test_1650_rntuple_writing_bugfixes.py @@ -0,0 +1,109 @@ +# BSD 3-Clause License; see https://github.com/scikit-hep/uproot5/blob/main/LICENSE + +""" +Regression tests for PR #1650: RNTuple footer leak on extend, broken +WritableNTuple.compression, directory __delitem__ KeyError. +""" + +from __future__ import annotations + +import numpy as np +import pytest + +import uproot +import uproot.exceptions + + +def test_rntuple_extend_no_freesegments_leak(tmp_path): + """ + Every call to WritableNTuple.extend() must fully release the old footer + region (key header + payload), not just the key header. + + Before the fix, NTuple.extend released only old_footer_key.allocation + (= num_bytes, ~70 bytes) instead of num_bytes + compressed_bytes. + Repeated extends left the footer payload stranded in FreeSegments, + causing permanent file bloat. + + We verify the fix by checking that the file size after N extends is + within a reasonable bound and that the file is still readable. + """ + fname = tmp_path / "rntuple_extend_leak.root" + f = uproot.recreate(fname) + f["t"] = {"x": np.array([1.0, 2.0, 3.0], dtype="f4")} + ntuple = f["t"] + n_extends = 10 + for i in range(n_extends): + ntuple.extend({"x": np.array([float(i), float(i + 1)], dtype="f4")}) + f.close() + + # File must be readable and contain all entries + with uproot.open(fname) as rf: + arr = rf["t"]["x"].array() + assert len(arr) == 3 + n_extends * 2 + + # Sanity check: file should not grow unboundedly. + # Without the fix, each extend leaked ~footer_size bytes that could never + # be reused. A simple bound: file should be under 20 kB for this tiny dataset. + size = fname.stat().st_size + assert size < 20_000, f"File unexpectedly large ({size} bytes), possible leak" + + +def test_writable_ntuple_compression_getter(tmp_path): + """ + WritableNTuple.compression must return the file-level compression object + without raising AttributeError (which it did before the fix because it + referenced self._cascading._branch_data which does not exist on NTuple). + """ + fname = tmp_path / "ntuple_compression.root" + f = uproot.recreate(fname, compression=uproot.ZLIB(1)) + f["t"] = {"x": np.array([1.0], dtype="f4")} + ntuple = f["t"] + comp = ntuple.compression + assert comp is not None + assert isinstance(comp, uproot.compression.Compression) + f.close() + + +def test_writable_ntuple_compression_setter_raises(tmp_path): + """ + WritableNTuple.compression setter must raise NotImplementedError (not + AttributeError) since per-RNTuple compression is not yet supported. + """ + fname = tmp_path / "ntuple_compression_set.root" + f = uproot.recreate(fname) + f["t"] = {"x": np.array([1.0], dtype="f4")} + ntuple = f["t"] + with pytest.raises(NotImplementedError): + ntuple.compression = uproot.ZLIB(1) + f.close() + + +def test_writable_directory_delitem_nonexistent_raises_key_error(tmp_path): + """ + Deleting a nonexistent key from a WritableDirectory must raise a KeyError + subclass (uproot.exceptions.KeyInFileError), not AttributeError. + + Before the fix, _del() called key.seek_location without checking whether + get_key() returned None, resulting in: + AttributeError: 'NoneType' object has no attribute 'seek_location' + """ + fname = tmp_path / "del_nonexistent.root" + f = uproot.recreate(fname) + f["t"] = {"x": np.array([1, 2, 3], dtype="i4")} + with pytest.raises(KeyError): + del f["nonexistent_key"] + f.close() + + +def test_writable_directory_delitem_nonexistent_is_key_in_file_error(tmp_path): + """ + The error raised for deleting a nonexistent key should specifically be + uproot.exceptions.KeyInFileError (a KeyError subclass), matching the + behaviour of __getitem__ (_get method). + """ + fname = tmp_path / "del_nonexistent2.root" + f = uproot.recreate(fname) + f["t"] = {"x": np.array([1], dtype="i4")} + with pytest.raises(uproot.exceptions.KeyInFileError): + del f["does_not_exist"] + f.close() From b9a9b5a99183f34e0f4764c8e7755bba93cde813 Mon Sep 17 00:00:00 2001 From: Andres Rios Tascon Date: Fri, 17 Jul 2026 10:24:40 -0400 Subject: [PATCH 3/3] Remove unnecessary tests --- tests/test_1650_rntuple_writing_bugfixes.py | 84 --------------------- 1 file changed, 84 deletions(-) diff --git a/tests/test_1650_rntuple_writing_bugfixes.py b/tests/test_1650_rntuple_writing_bugfixes.py index 2e2001a25..d2657c85a 100644 --- a/tests/test_1650_rntuple_writing_bugfixes.py +++ b/tests/test_1650_rntuple_writing_bugfixes.py @@ -1,81 +1,11 @@ # BSD 3-Clause License; see https://github.com/scikit-hep/uproot5/blob/main/LICENSE -""" -Regression tests for PR #1650: RNTuple footer leak on extend, broken -WritableNTuple.compression, directory __delitem__ KeyError. -""" - from __future__ import annotations import numpy as np import pytest import uproot -import uproot.exceptions - - -def test_rntuple_extend_no_freesegments_leak(tmp_path): - """ - Every call to WritableNTuple.extend() must fully release the old footer - region (key header + payload), not just the key header. - - Before the fix, NTuple.extend released only old_footer_key.allocation - (= num_bytes, ~70 bytes) instead of num_bytes + compressed_bytes. - Repeated extends left the footer payload stranded in FreeSegments, - causing permanent file bloat. - - We verify the fix by checking that the file size after N extends is - within a reasonable bound and that the file is still readable. - """ - fname = tmp_path / "rntuple_extend_leak.root" - f = uproot.recreate(fname) - f["t"] = {"x": np.array([1.0, 2.0, 3.0], dtype="f4")} - ntuple = f["t"] - n_extends = 10 - for i in range(n_extends): - ntuple.extend({"x": np.array([float(i), float(i + 1)], dtype="f4")}) - f.close() - - # File must be readable and contain all entries - with uproot.open(fname) as rf: - arr = rf["t"]["x"].array() - assert len(arr) == 3 + n_extends * 2 - - # Sanity check: file should not grow unboundedly. - # Without the fix, each extend leaked ~footer_size bytes that could never - # be reused. A simple bound: file should be under 20 kB for this tiny dataset. - size = fname.stat().st_size - assert size < 20_000, f"File unexpectedly large ({size} bytes), possible leak" - - -def test_writable_ntuple_compression_getter(tmp_path): - """ - WritableNTuple.compression must return the file-level compression object - without raising AttributeError (which it did before the fix because it - referenced self._cascading._branch_data which does not exist on NTuple). - """ - fname = tmp_path / "ntuple_compression.root" - f = uproot.recreate(fname, compression=uproot.ZLIB(1)) - f["t"] = {"x": np.array([1.0], dtype="f4")} - ntuple = f["t"] - comp = ntuple.compression - assert comp is not None - assert isinstance(comp, uproot.compression.Compression) - f.close() - - -def test_writable_ntuple_compression_setter_raises(tmp_path): - """ - WritableNTuple.compression setter must raise NotImplementedError (not - AttributeError) since per-RNTuple compression is not yet supported. - """ - fname = tmp_path / "ntuple_compression_set.root" - f = uproot.recreate(fname) - f["t"] = {"x": np.array([1.0], dtype="f4")} - ntuple = f["t"] - with pytest.raises(NotImplementedError): - ntuple.compression = uproot.ZLIB(1) - f.close() def test_writable_directory_delitem_nonexistent_raises_key_error(tmp_path): @@ -93,17 +23,3 @@ def test_writable_directory_delitem_nonexistent_raises_key_error(tmp_path): with pytest.raises(KeyError): del f["nonexistent_key"] f.close() - - -def test_writable_directory_delitem_nonexistent_is_key_in_file_error(tmp_path): - """ - The error raised for deleting a nonexistent key should specifically be - uproot.exceptions.KeyInFileError (a KeyError subclass), matching the - behaviour of __getitem__ (_get method). - """ - fname = tmp_path / "del_nonexistent2.root" - f = uproot.recreate(fname) - f["t"] = {"x": np.array([1], dtype="i4")} - with pytest.raises(uproot.exceptions.KeyInFileError): - del f["does_not_exist"] - f.close()