From 8303aa458f1360164d7d731a8aa43ceb2a5c21e5 Mon Sep 17 00:00:00 2001 From: YohannMrg <98689112+yohannmarguier@users.noreply.github.com> Date: Tue, 21 Jul 2026 13:41:57 +0200 Subject: [PATCH 1/3] Fix HDF5 dataset re-creation on repeated whole-IDS writes to an open pulse (issue #60) HDF5Writer::write_ND_Data and ::createOrUpdateShapesDataSet decided create-vs-reuse for a dataset purely from the per-action opened_data_sets cache, which is cleared at the end of every al_end_action(WRITE_OP) even though the underlying HDF5 file (and its datasets) persists for as long as the pulse stays open. A second global write on the same still-open pulse therefore always took the "not yet created" branch and called H5Dcreate2 on a dataset that already existed, throwing "Unable to create HDF5 dataset". Both call sites now check H5Lexists first and open() the existing dataset instead of re-creating it, mirroring the SLICE_OP branch a few lines below (and createOrUpdateAOSShapesDataSet, a third function that already had this guard). Adds EquilibriumSeedMatrix.RepeatedWholeIdsWriteOnSameOpenPulseSucceeds to pin the fix: two al_begin_global_action(WRITE_OP) calls on the same open pulse, reproducing the exact failure shape independent of which benchmark or caller triggers it. Verified by reverting the fix alone and confirming the new test fails with the original error message. (cherry picked from commit 353a66d0d3b2a758b8cd3cac42e8e79628e37825) --- src/hdf5/hdf5_writer.cpp | 16 +++++++++--- tests/contract/test_structured_data.cpp | 33 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/hdf5/hdf5_writer.cpp b/src/hdf5/hdf5_writer.cpp index 57d8f702..477fd514 100644 --- a/src/hdf5/hdf5_writer.cpp +++ b/src/hdf5/hdf5_writer.cpp @@ -414,12 +414,17 @@ void HDF5Writer::write_ND_Data(Context * ctx, std::string & att_name, std::strin if (slice_mode != SLICE_OP) { //std::cout << "WRITER NOT IN SLICE MODE!!! " << std::endl; - if (dataset_id < 0) //in global mode (put), data set not yet created + if (dataset_id < 0) //not in this action's opened_data_sets cache -- may still exist on disk from a prior action on the same open pulse { std::unique_ptr < HDF5DataSetHandler > dataSetHandler(new HDF5DataSetHandler(true, dec->getURI())); dataSetHandler->setNonSliceMode(); bool create_chunk_cache = true; - dataSetHandler->create(tensorized_path.c_str(), &dataset_id, datatype, gid, dim, size, AOSRank, arrctx_shapes.data(), false, create_chunk_cache); + if (H5Lexists(gid, tensorized_path.c_str(), H5P_DEFAULT) == 0) { + dataSetHandler->create(tensorized_path.c_str(), &dataset_id, datatype, gid, dim, size, AOSRank, arrctx_shapes.data(), false, create_chunk_cache); + } else { + dataSetHandler->open(tensorized_path.c_str(), gid, &dataset_id, dim, size, datatype, false, create_chunk_cache, dec->getURI(), AOSRank, arrctx_shapes.data()); + dataSetHandler->setCurrentShapesAndExtend(size, arrctx_shapes.data()); + } data_set = std::move(dataSetHandler); } else { data_set = std::move(got->second); @@ -608,7 +613,12 @@ std::string & timebasename, int timed_AOS_index, const std::vector < int > &curr } else { data_set->setNonSliceMode(); bool create_chunk_cache = true; - data_set->create(tensorized_path.c_str(), &dataset_id, alconst::integer_data, loc_id, dim, size, AOSRank, aos_shapes.data(), shapes_dataset, create_chunk_cache); + if (H5Lexists(loc_id, tensorized_path.c_str(), H5P_DEFAULT) == 0) { + data_set->create(tensorized_path.c_str(), &dataset_id, alconst::integer_data, loc_id, dim, size, AOSRank, aos_shapes.data(), shapes_dataset, create_chunk_cache); + } else { + data_set->open(tensorized_path.c_str(), loc_id, &dataset_id, dim, size, alconst::integer_data, shapes_dataset, create_chunk_cache, dec->getURI(), AOSRank, aos_shapes.data()); + data_set->setCurrentShapesAndExtend(size, aos_shapes.data()); + } } if (data_set->useBuffering && slice_mode != SLICE_OP) { diff --git a/tests/contract/test_structured_data.cpp b/tests/contract/test_structured_data.cpp index 86029d7f..a625fbad 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -367,6 +367,39 @@ TEST_P(EquilibriumSeedMatrix, RoundTripHashMatches) { EXPECT_EQ(al_close_pulse(pctx, CLOSE_PULSE).code, 0); } +// A second whole-IDS write on the *same still-open* pulse must succeed and +// overwrite in place, not fail as if the underlying storage were being +// created for the first time (issue #60 follow-up: HDF5Writer::write_ND_Data +// and ::createOrUpdateShapesDataSet decided create-vs-reuse purely from their +// own per-action opened_data_sets cache -- which is cleared at the end of +// every al_begin_global_action/al_end_action pair -- instead of asking the +// file itself via H5Lexists, so any second global WRITE_OP on an already- +// populated open pulse re-attempted H5Dcreate2 on a dataset that already +// existed and threw "Unable to create HDF5 dataset"). Two independent +// al_begin_global_action(WRITE_OP) calls on one pctx reproduce the exact +// shape of the failure regardless of what benchmark or caller triggers it. +TEST_P(EquilibriumSeedMatrix, RepeatedWholeIdsWriteOnSameOpenPulseSucceeds) { + const BackendCase b = GetParam(); + if (b.on_disk) base_.make_legacy_tree(pulse_); + const std::string uri = al_contract::build_uri(b.id, base_.str(), pulse_); + ASSERT_FALSE(uri.empty()); + + int pctx = -1; + AL_ASSERT_OK(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx)); + AL_ASSERT_OK(equilibrium_seed::write(pctx)); + AL_ASSERT_OK(equilibrium_seed::write(pctx)) + << "a second whole-IDS write on the same still-open pulse must " + "overwrite in place, not fail as if the dataset didn't exist yet"; + + std::vector obs; + AL_ASSERT_OK(equilibrium_seed::read_back(pctx, &obs)); + EXPECT_EQ(equilibrium_seed::canonical_hash(obs), + equilibrium_seed::expected_hash()) + << "repeated writes must never accumulate stale data"; + + EXPECT_EQ(al_close_pulse(pctx, CLOSE_PULSE).code, 0); +} + // --- structural-hash sensitivity (issue #33 acceptance criteria 2 & 3) ------ // Hermetic proofs that the canonical hash catches the two structural // corruptions a values-only hash would miss. They operate on the production From ff3aed54e3b0e6711cb9249184cb778280e320d1 Mon Sep 17 00:00:00 2001 From: YohannMrg <98689112+yohannmarguier@users.noreply.github.com> Date: Tue, 21 Jul 2026 15:26:47 +0200 Subject: [PATCH 2/3] Cover repeated HDF5 dataset writes (cherry picked from commit 8ab1815c156f0f2faa1ed3f7eeb93fc258dca900) --- src/hdf5/hdf5_writer.cpp | 46 +++++++++++++++---------- src/hdf5/hdf5_writer.h | 5 +++ tests/contract/test_structured_data.cpp | 45 ++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 18 deletions(-) diff --git a/src/hdf5/hdf5_writer.cpp b/src/hdf5/hdf5_writer.cpp index 477fd514..b03aa0cc 100644 --- a/src/hdf5/hdf5_writer.cpp +++ b/src/hdf5/hdf5_writer.cpp @@ -339,6 +339,28 @@ ArraystructContext* HDF5Writer::getDynamicAOS(Context * ctx) { return NULL; } +std::unique_ptr HDF5Writer::openOrCreateNonSliceDataSet( + const std::string& tensorized_path, hid_t* dataset_id, int datatype, + hid_t location, int dim, int* size, int aos_rank, + int* aos_shapes, bool shapes_dataset, + DataEntryContext* dataentry_context) { + auto data_set = std::make_unique( + true, dataentry_context->getURI()); + data_set->setNonSliceMode(); + constexpr bool create_chunk_cache = true; + if (H5Lexists(location, tensorized_path.c_str(), H5P_DEFAULT) == 0) { + data_set->create(tensorized_path.c_str(), dataset_id, datatype, + location, dim, size, aos_rank, aos_shapes, + shapes_dataset, create_chunk_cache); + } else { + data_set->open(tensorized_path.c_str(), location, dataset_id, dim, + size, datatype, shapes_dataset, create_chunk_cache, + dataentry_context->getURI(), aos_rank, aos_shapes); + data_set->setCurrentShapesAndExtend(size, aos_shapes); + } + return data_set; +} + void HDF5Writer::write_ND_Data(Context * ctx, std::string & att_name, std::string & timebasename, int datatype, int dim, int *size, void *data) { @@ -416,16 +438,9 @@ void HDF5Writer::write_ND_Data(Context * ctx, std::string & att_name, std::strin //std::cout << "WRITER NOT IN SLICE MODE!!! " << std::endl; if (dataset_id < 0) //not in this action's opened_data_sets cache -- may still exist on disk from a prior action on the same open pulse { - std::unique_ptr < HDF5DataSetHandler > dataSetHandler(new HDF5DataSetHandler(true, dec->getURI())); - dataSetHandler->setNonSliceMode(); - bool create_chunk_cache = true; - if (H5Lexists(gid, tensorized_path.c_str(), H5P_DEFAULT) == 0) { - dataSetHandler->create(tensorized_path.c_str(), &dataset_id, datatype, gid, dim, size, AOSRank, arrctx_shapes.data(), false, create_chunk_cache); - } else { - dataSetHandler->open(tensorized_path.c_str(), gid, &dataset_id, dim, size, datatype, false, create_chunk_cache, dec->getURI(), AOSRank, arrctx_shapes.data()); - dataSetHandler->setCurrentShapesAndExtend(size, arrctx_shapes.data()); - } - data_set = std::move(dataSetHandler); + data_set = openOrCreateNonSliceDataSet( + tensorized_path, &dataset_id, datatype, gid, dim, size, + AOSRank, arrctx_shapes.data(), false, dec); } else { data_set = std::move(got->second); opened_data_sets.erase(got); @@ -611,14 +626,9 @@ std::string & timebasename, int timed_AOS_index, const std::vector < int > &curr } } else { - data_set->setNonSliceMode(); - bool create_chunk_cache = true; - if (H5Lexists(loc_id, tensorized_path.c_str(), H5P_DEFAULT) == 0) { - data_set->create(tensorized_path.c_str(), &dataset_id, alconst::integer_data, loc_id, dim, size, AOSRank, aos_shapes.data(), shapes_dataset, create_chunk_cache); - } else { - data_set->open(tensorized_path.c_str(), loc_id, &dataset_id, dim, size, alconst::integer_data, shapes_dataset, create_chunk_cache, dec->getURI(), AOSRank, aos_shapes.data()); - data_set->setCurrentShapesAndExtend(size, aos_shapes.data()); - } + data_set = openOrCreateNonSliceDataSet( + tensorized_path, &dataset_id, alconst::integer_data, loc_id, + dim, size, AOSRank, aos_shapes.data(), shapes_dataset, dec); } if (data_set->useBuffering && slice_mode != SLICE_OP) { diff --git a/src/hdf5/hdf5_writer.h b/src/hdf5/hdf5_writer.h index 70caf417..727e496b 100644 --- a/src/hdf5/hdf5_writer.h +++ b/src/hdf5/hdf5_writer.h @@ -32,6 +32,11 @@ class HDF5Writer { int getDynamic_AOS_slices_extension(Context *ctx); int getDynamic_slices_extension(Context *ctx, int timed_AOS_index, int time_vector_length); ArraystructContext* getDynamicAOS(Context * ctx); + std::unique_ptr openOrCreateNonSliceDataSet( + const std::string& tensorized_path, hid_t* dataset_id, int datatype, + hid_t location, int dim, int* size, int aos_rank, + int* aos_shapes, bool shapes_dataset, + DataEntryContext* dataentry_context); public: diff --git a/tests/contract/test_structured_data.cpp b/tests/contract/test_structured_data.cpp index a625fbad..d49937e5 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -400,6 +400,51 @@ TEST_P(EquilibriumSeedMatrix, RepeatedWholeIdsWriteOnSameOpenPulseSucceeds) { EXPECT_EQ(al_close_pulse(pctx, CLOSE_PULSE).code, 0); } +// Reproduce the shared benchmark process ordering from issue #60: a complete +// whole-IDS read, followed by the node-by-node read of one time_slice element. +// This keeps the smoke-only HDF5 failure covered without relying on benchmark +// registration order or a standalone filtered process. +TEST_P(EquilibriumSeedMatrix, WholeIdsReadThenTimeSliceElementReadSucceeds) { + const BackendCase b = GetParam(); + if (b.on_disk) base_.make_legacy_tree(pulse_); + const std::string uri = al_contract::build_uri(b.id, base_.str(), pulse_); + ASSERT_FALSE(uri.empty()); + + int pctx = -1; + AL_ASSERT_OK(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx)); + AL_ASSERT_OK(equilibrium_seed::write(pctx)); + + std::vector whole_ids; + AL_ASSERT_OK(equilibrium_seed::read_back(pctx, &whole_ids)); + + int op = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, equilibrium_seed::kIds, "", + READ_OP, &op)); + int size = 0; + int aos_ctx = -1; + AL_ASSERT_OK(al_begin_arraystruct_action(op, equilibrium_seed::kAos, + equilibrium_seed::kAosTime, + &size, &aos_ctx)); + ASSERT_EQ(size, equilibrium_seed::kNSlices); + AL_ASSERT_OK(al_iterate_over_arraystruct(aos_ctx, size - 1)); + + std::vector shape; + std::vector data; + auto read_leaf = [&](const char* path, int rank) { + AL_ASSERT_OK(al_contract::read_data(aos_ctx, path, rank, + &shape, &data)); + }; + read_leaf(equilibrium_seed::kSliceTime, 0); + read_leaf(equilibrium_seed::kPsi, 1); + read_leaf(equilibrium_seed::kIp, 0); + read_leaf(equilibrium_seed::kMeasured, 0); + read_leaf(equilibrium_seed::kWeight, 0); + + AL_ASSERT_OK(al_end_action(aos_ctx)); + AL_ASSERT_OK(al_end_action(op)); + EXPECT_EQ(al_close_pulse(pctx, CLOSE_PULSE).code, 0); +} + // --- structural-hash sensitivity (issue #33 acceptance criteria 2 & 3) ------ // Hermetic proofs that the canonical hash catches the two structural // corruptions a values-only hash would miss. They operate on the production From 061bad9476966a2fb15c41bfdde5b74682150814 Mon Sep 17 00:00:00 2001 From: YohannMrg <98689112+yohannmarguier@users.noreply.github.com> Date: Tue, 21 Jul 2026 15:38:58 +0200 Subject: [PATCH 3/3] Skip ASCII in AOS traversal regression (cherry picked from commit 36f4cdc4c540aa0292099ad421aff5b104c9568a) --- tests/contract/test_structured_data.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/contract/test_structured_data.cpp b/tests/contract/test_structured_data.cpp index d49937e5..d945aeaa 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -406,6 +406,14 @@ TEST_P(EquilibriumSeedMatrix, RepeatedWholeIdsWriteOnSameOpenPulseSucceeds) { // registration order or a standalone filtered process. TEST_P(EquilibriumSeedMatrix, WholeIdsReadThenTimeSliceElementReadSucceeds) { const BackendCase b = GetParam(); + // ASCII's READ_OP setup exhausts its sequential stream before + // beginReadArraystructAction can recover the AOS size. This is the + // documented AOS read known defect pinned by AosKnownDefects.* above; + // this regression specifically covers the HDF5 benchmark ordering. + if (b.id == ASCII_BACKEND) { + GTEST_SKIP() << "known defect for " << b.name + << " AOS read — see AosKnownDefects.*"; + } if (b.on_disk) base_.make_legacy_tree(pulse_); const std::string uri = al_contract::build_uri(b.id, base_.str(), pulse_); ASSERT_FALSE(uri.empty());