Ragged write leaves partial data when the index dtype is not uint32 #332

Note on provenance: This issue was found by Claude Opus 5 while reviewing #325. The reproductions and output shown below were produced by building and running the code, but I have only briefly reviewed the analysis myself and I am not an expert in this area. Please review it critically.

Impact

addRows can only write a ragged column whose index dataset is uint32. For any other unsigned width it writes the row's values to the target dataset first, then fails on the index, and returns Status::Failure over a file it has already modified. There is no rollback.

So a caller who correctly checks the return value and sees Failure will reasonably assume nothing was written. In fact the target column has grown, while the index and id have not, leaving the table inconsistent in the same way as a ragged corruption.

Today this is hard to reach, because every built in spec uses uint32. It becomes easy to reach the moment #325's reopen path is fixed, which is the reason to fix the two together. HDMF starts every VectorIndex at uint8 and only widens it as the data grows (hdmf/common/table.py:134-169), so a PyNWB written table whose ragged column holds fewer than 256 elements has a uint8 index, and fewer than 65,536 gives uint16. Those are ordinary files. uint32 only appears past 65,535 elements.

What happens

One addRow with a two element ragged cell, varying only the index dtype:

Confirmed on disk with h5dump:

DATASET "id"                { DATASPACE SIMPLE { ( 0 ) / ( H5S_UNLIMITED ) } }
DATASET "ragged_data"       { DATATYPE H5T_STD_I32LE  DATASPACE SIMPLE { ( 2 ) / ( H5S_UNLIMITED ) } }
DATASET "ragged_data_index" { DATATYPE H5T_STD_U64LE  DATASPACE SIMPLE { ( 0 ) / ( H5S_UNLIMITED ) } }

The emitted diagnostic is VectorData::appendBuffer: buffer type does not match dataset type.

Cause

addRows hardcodes the index accumulation buffer as std::vector<uint32_t>, while VectorIndex::initialize explicitly accepts T_U8, T_U16, T_U32 and T_U64. isCompatibleVector is an exact variant match, so anything but uint32 is rejected, and the rejection happens after the target write has already been committed.

std::unordered_map<std::string, std::vector<uint32_t>> indexBuffers;

createEmptyVectorVariant(target->readData()->getDataType())

Suggested direction

Derive the index buffer variant from the index column's own dtype, exactly as the target buffer already does. That covers all four widths with no new policy.

Separately worth considering: the partial commit window at :571-591 is a hazard for any I/O failure on the index write, not only a dtype mismatch. Writing the index before the target, or making the row write recoverable, would close the general case.

Notes

The existing ragged test at tests/testDynamicTable.cpp:855-950 reaches this code and passes only because it happens to use U32. Parameterising it over the four accepted widths would have caught this.

Related

Blocked by #330, and must be fixed together with it. This issue is hard to reach today only because #330 masks it: loadConfiguredColumnsFromFile never restores a VectorIndex, so addRows never attempts an index write on a reopened table. Closing #330 without also fixing this one converts the common PyNWB interop case from one corruption mode into another.