GPT 5.6 Sol review of Uproot

[ariostas]

Following #1646, I thought it would be worth reviewing the repo with GPT 5.6 Sol to see if it found a new set of issues. I don't know if they are as trust-worthy as the Fable ones, but I'll take a look at them.

🤖 AI text below 🤖

This is a follow-up to #1646 after its associated fixes were merged. The review was performed at commit 5388b85 and focused on write integrity, source/executor lifecycle, read-model correctness, and multi-file performance.

Several findings were reproduced directly. The existing focused source tests still pass, which indicates missing regression coverage rather than known failing tests:

uv run pytest tests/test_1654_source_sink_misc.py tests/test_1653_http_source_multipart_batching.py tests/test_1650_rntuple_writing_bugfixes.py -q
13 passed

Suggested PR grouping

The findings seem manageable as seven focused PRs. The groupings keep shared invariants and test fixtures together while avoiding one large cross-cutting change.

PR 1: sink and writer integrity

1. Directory allocations smaller than four bytes corrupt files (writing/writable.py:119-124, writing/_cascade.py:1237-1256, writing/_cascade.py:2295-2318). initial_directory_bytes=1 or 3 creates a file that cannot be reopened for update; 0 and negative values fail later with unrelated IndexErrors. Validate the public root and nested-directory arguments against _directory_format_num_keys.size, and defensively reject serializations larger than their allocation.
2. Partial writes are treated as complete (sink/file.py:162-185, writing/_cascade.py:96-106). A valid file-like object whose write accepts three bytes caused the cascade to mark a ten-byte record clean. FileSink.write and set_file_length need write-all semantics or an explicit error on zero progress.
3. recreate does not physically truncate existing files (sink/file.py:53-58, sink/file.py:175-185, writing/writable.py:86-141). Recreating a 100 KB path as an empty ROOT file left the physical size at 100 KB while fEND was 1,682. recreate should use a truncating mode; update should retain r+b behavior.

Suggested tests: partial-writing BytesIO, root/nested directories with -1, 0, 1, 3, 4, and physical file-size/marker checks after recreate.

PR 2: transactional and scalable RNTuple extension

1. RBlob flushes expose stale FreeSegments metadata (writing/_cascadentuple.py:936-986, writing/_cascade.py:667-675). At EOF, allocation moves the FreeSegments record in memory and writes an RBlob over its old persisted location. add_rblob then flushes before the relocated FreeSegments record is written. A process or storage failure in this window leaves fSeekFree pointing into an RBlob, so a later update can overwrite RNTuple data.
2. Repeated extension rewrites cumulative footer metadata and flushes each blob (writing/_cascadentuple.py:502-534, writing/_cascadentuple.py:932-986). Every extension serializes all prior cluster-group records, giving quadratic cumulative metadata work, while per-column/blob flushes are especially expensive for remote sinks.

Suggested implementation order: prepare and validate pages, persist the relocated FreeSegments state, publish the new anchor last, and batch flushing at the commit boundary. Add a fault-injecting sink test plus instrumentation for footer bytes and flush counts over repeated extensions.

PR 3: futures, chunk validation, and context-manager lifecycle

1. Shutdown races can orphan futures forever (source/futures.py:236-256, source/futures.py:367-407). ResourceThreadPoolExecutor does not mark itself closed until after sentinels are queued and workers joined. A concurrent submit can be accepted behind a sentinel; this was reproduced as a future that timed out permanently.
2. Executor kwargs are accepted but discarded (source/futures.py:112-143, source/futures.py:236-245). A keyword-only task submitted with x=3 ran without the keyword and raised TypeError.
3. Failed chunk-length validation only fails once (source/chunk.py:376-415). A chunk declared as five bytes but backed by three raised on first raw_data access and returned the truncated bytes on the second.
4. Read context managers do not shut down attached executors (reading.py:646-667, reading.py:1524-1529, model.py:400-407). Both directory and attached-model __exit__ methods close only the source, despite open documenting that attached executors are shut down when the file closes. Recording executors observed zero shutdown calls.

A small explicit executor state machine (RUNNING, SHUTTING_DOWN, CLOSED) or using standard concurrent.futures semantics would simplify these contracts. Tests should cover submit-versus-close races, kwargs, repeated short-chunk access, and both forms of with uproot.open(...).

PR 4: HTTP redirects and XRootD callback robustness

1. HTTP redirects forward Basic credentials to a different origin (source/http.py:97-117, source/http.py:203-216). Redirect requests reuse authorization derived from the original URL. Redirect handling also parses Location without urljoin, so relative redirects fail, and has no redirect limit/cycle detection.
2. Partial or malformed successful XRootD vector responses leave futures pending (source/xrootd.py:235-254). A synthetic successful (0, 5) response for a requested (0, 10) range raised KeyError in the callback and left the original future unresolved. Missing, short, unexpected, and duplicate ranges should complete every affected future with an exception.

Suggested tests: two local HTTP origins to verify credential stripping, relative and cyclic redirects, and callback tests asserting that all XRootD futures reach a terminal state for malformed responses.

PR 5: TTree schema, interpretation caches, and unknown models

1. Generated TTree counters invalidate branch lookup indices (writing/_cascadetree.py:198-207). When a generated counter name already exists, its datum is deleted without decrementing later lookup indices. A schema with scalar nx, scalar y, and jagged x produced {"nx": 1, "y": 1} and failed extension. Replace in place or rebuild all indices after collision handling.
2. Structured AsDtype cache keys omit offsets and total item size (interpretation/numerical.py:283-317, behaviors/TBranch.py:1256-1259, behaviors/TBranch.py:3216-3224). Packed fields at offsets [0, 4], itemsize 8, and padded fields at [0, 8], itemsize 12 generated identical cache keys. This can reuse an array interpreted with a different physical layout and bypasses the multiple-interpretation guard.
3. Unknown model classes are cached only by classname (model.py:1281-1290, reading.py:1132-1172). Requesting unknown versions 1 and 2 returned the same Unknown_X_v1 class. The cache key needs to include version and distinguish versioned from versionless unknown classes.

These share compact synthetic regression tests and do not require external ROOT files.

PR 6: Pandas grouping and global indices

1. Pandas how=dict fails in multi-file APIs (interpretation/library.py:917-939, behaviors/TBranch.py:235-245, behaviors/TBranch.py:466). Both uproot.concatenate(..., library="pd", how=dict) and top-level uproot.iterate raise AttributeError: 'dict' object has no attribute 'index', even with one file. global_index should recurse through dictionaries as it already does for tuples and lists.
2. RNTuple Pandas indices become cluster-relative (behaviors/RNTuple.py:855-903). Reading entries [4:6] across two written clusters returned the correct values with index [1, 2]. Preserve the normalized global range before subtracting the cluster offset.

Suggested tests should cover one and multiple files for how=dict, plus entry ranges inside and across nonzero RNTuple clusters.

PR 7: multi-file scaling and partition semantics

1. concatenate opens every input before reading any input (behaviors/TBranch.py:403-416). all_hasbranches retains every source while computing the total entry count, which can exhaust descriptors, sockets, and source worker pools for large globs. Positive bounded ranges can stream; negative ranges may need a metadata-only first pass followed by reopening.
2. Dask averages per-tree steps into one global entry step (_dask.py:691-725, _dask.py:1700-1735). Two trees with 2 and 10 entries and steps_per_file=2 produced five partitions with chunks (2, 3, 3, 3, 1), not two partitions per file. For memory-sized step_size, averaging can also make partitions from expensive trees exceed the requested memory target.

The Dask partition-count behavior overlaps existing issue #1273; implementation should be tracked there and linked back here rather than opening another issue. Add heterogeneous file sizes and jagged payloads to its regression coverage.

Priority suggestion

Recommended order is PR 1, PR 3, PR 4, PR 2, PR 5, PR 6, PR 7. That addresses deterministic corruption and indefinite hangs first, then crash consistency and user-facing correctness, with scaling changes last because they may require more API discussion.

No tracked repository files were modified during this review.