From 61cf1689e800a945516b4df028631cb7f8d3b3b7 Mon Sep 17 00:00:00 2001 From: YohannMrg <98689112+yohannmarguier@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:01:14 +0200 Subject: [PATCH 1/4] Pin HDF5 al_delete_data's occurrence-wide widening (issue #63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `al_delete_data`'s `path` is accepted by the ABI and dropped by the HDF5 backend: `HDF5Backend::deleteData` never forwards it and `HDF5Writer:: deleteData` has no parameter to receive it, so any non-empty path removes the whole IDS occurrence and reports success. Characterize that before touching it. The existing tripwire only observed the sibling leaf; widen it to the two consequences issue #63 measured as well — the occurrence's own `.h5` is deleted, and the master file is left holding an external link to a file that no longer exists (observed through `al_get_occurrences`, which enumerates those links live). Confirmed red/green today: the three correct-contract assertions fail, the tripwire passes. (cherry picked from commit 5748ec5329a08e215421c3002173117685710d32) --- tests/contract/TRACEABILITY.md | 3 +- tests/contract/test_structured_data.cpp | 82 +++++++++++++++++++++---- 2 files changed, 71 insertions(+), 14 deletions(-) diff --git a/tests/contract/TRACEABILITY.md b/tests/contract/TRACEABILITY.md index 9a59e6e7..23be3fdd 100644 --- a/tests/contract/TRACEABILITY.md +++ b/tests/contract/TRACEABILITY.md @@ -128,7 +128,7 @@ genuine defect (expected-fail). | ↳ CHAR scalar (dim 0) must round-trip — **HDF5 crashes** | :244-263 | — | ✓ | `RoundTripKnownDefects.DISABLED_Hdf5CharScalarRoundTrips` + tripwire `RoundTripKnownDefectsDeath.Hdf5CharScalarCurrentlyCrashes` | **xfail** | | ↳ numeric at MAXDIM (rank 7) must round-trip — **ASCII corrupts (DOUBLE/COMPLEX) / aborts (INTEGER)** | :244-263 | — | ✓ | `RoundTripKnownDefects.DISABLED_Ascii{Integer,Double,Complex}MaxdimRoundTrips` + tripwires `RoundTripKnownDefectsDeath.AsciiIntegerMaxdimCurrentlyAborts`, `RoundTripKnownDefectsDeath.Ascii{Double,Complex}MaxdimCurrentlyCorrupts` (death-test children: corruption *or* crash keeps them green — the overrun's manifestation is machine-dependent) | **xfail** | | `al_delete_data` at signal / structure / DATAOBJECT-root granularity | :265-272 | ✓ (Memory) | ✓ (HDF5) | Support is disjoint per backend, none implement all three (see xfail rows below): `Backends/DeleteMatrix.{LeafDeleteRemovesJustTheLeaf,StructureDeleteRemovesWholeSubtree}/Memory` (leaf + structure genuinely work only on Memory), `…/RootDeleteRemovesWholeOccurrence/HDF5` (root genuinely works only on HDF5, which ignores `path` and always removes the whole occurrence). MDSplus: not joined to this fixture — terminal gap, see Part 4 (issue #18). UDA: remote delete is cleanly refused (the reference plugin has no `deleteData` handler); remote write over the same path falsely reports success instead of the same refusal — xfail, see Part 5 | covered | -| ↳ HDF5 ignores `path` for leaf/structure delete — **any delete call removes the whole occurrence**, not just the named field | :265-272; src/hdf5/hdf5_writer.cpp deleteData (no path parameter at all) | — | ✓ | One tripwire covers both defective granularities, since there is no leaf-vs-structure branch to differ (`HDF5Writer::deleteData` never receives a path at all): `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteLeavesSiblingIntact` + tripwire `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence`; matrix cells skip via `Backends/DeleteMatrix.{Leaf,Structure}*/HDF5` | **xfail** | +| ↳ HDF5 ignores `path` for leaf/structure delete — **any delete call removes the whole occurrence**, not just the named field | :265-272; src/hdf5/hdf5_writer.cpp deleteData (no path parameter at all) | — | ✓ | One tripwire covers both defective granularities, since there is no leaf-vs-structure branch to differ (`HDF5Writer::deleteData` never receives a path at all): `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteLeavesSiblingIntact` + `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink` + tripwire `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence`, which pins all three observable consequences (sibling gone, `.h5` deleted, master file left holding a dangling external link — issue #63); matrix cells skip via `Backends/DeleteMatrix.{Leaf,Structure}*/HDF5` | **xfail** | | ↳ Memory has no code path for DATAOBJECT-root delete — **a root-granularity delete is silently a no-op** | :265-272; src/memory_backend.cpp deleteData (no root/occurrence case) | ✓ | — | `DeleteKnownDefects.DISABLED_MemoryRootDeleteClearsWholeIds` + tripwire `DeleteKnownDefects.MemoryRootDeleteCurrentlyDoesNothing`; matrix cell skips via `Backends/DeleteMatrix.RootDeleteRemovesWholeOccurrence/Memory` | **xfail** | | ↳ ASCII's `deleteData` is a literal no-op (empty body) at every granularity; Flexbuffers' is too but is not independently ABI-observable (it already refuses every in-session read regardless of delete, so a read-based oracle can't distinguish "delete did nothing" from "this backend never round-trips reads") | :265-272; src/ascii_backend.cpp:648-652, src/flexbuffers_backend.cpp:387-389 | ✓ | ✓ | One tripwire covers every granularity on ASCII, since an empty function body has no path-dependent branch to differ: `DeleteKnownDefects.DISABLED_AsciiDeleteRemovesLeaf` + tripwire `DeleteKnownDefects.AsciiDeleteIsCurrentlyANoOp`; matrix cells skip via `Backends/DeleteMatrix.*/ASCII` and `…/Flexbuffers` | **xfail** | | `al_iterate_over_arraystruct` advances the AOS cursor | :274-282 | ✓ (Memory) | ✓ (HDF5) | Exercised as an asserted dependency of every AOS write/read in `Backends/AosMatrix.*` (top-level and nested, multi-step) | covered (indirect) | @@ -276,6 +276,7 @@ whoever fixed it to enable the paired `DISABLED_` correct-contract test: | `al_get_backendID` accepts a non-pulse context via unchecked `static_cast` | `GetBackendIdKnownDefects.DISABLED_WrongContextTypeReturnsError` | `GetBackendIdKnownDefects.WrongContextTypeCurrentlySucceedsViaUnsafeCast` | | ASCII AOS read always reports size 0 | `AosKnownDefects.DISABLED_AsciiAosReadReportsWrittenSize` | `AosKnownDefects.AsciiAosReadCurrentlyReportsZero` | | HDF5 leaf/structure delete wipes the whole occurrence | `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteLeavesSiblingIntact` | `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence` | +| HDF5 leaf delete deletes `.h5` and leaves a dangling master-file link (issue #63) | `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink` | `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence` | | Memory root delete is a no-op | `DeleteKnownDefects.DISABLED_MemoryRootDeleteClearsWholeIds` | `DeleteKnownDefects.MemoryRootDeleteCurrentlyDoesNothing` | | ASCII delete is a no-op at every granularity | `DeleteKnownDefects.DISABLED_AsciiDeleteRemovesLeaf` | `DeleteKnownDefects.AsciiDeleteIsCurrentlyANoOp` | diff --git a/tests/contract/test_structured_data.cpp b/tests/contract/test_structured_data.cpp index 86029d7f..b2e2d63a 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include @@ -611,8 +612,34 @@ const BackendCase kHdf5{HDF5_BACKEND, "HDF5", /*on_disk=*/true}; const BackendCase kAscii{ASCII_BACKEND, "ASCII", /*on_disk=*/true}; // --- HDF5: leaf delete ignores `path`, wipes the whole occurrence ---------- -double hdf5_leaf_delete_sibling_survives(al_contract::TempBase& base, - const PulseId& pulse) { +// One non-empty-path delete of a single leaf, observed on three axes: the +// sibling leaf, the occurrence's own pulse file, and the external link the +// master file holds for that occurrence. The last two are what issue #63 +// measured, and they are why the widening is not merely untidy — the call +// reports success while removing the whole occurrence, and leaves master.h5 +// pointing at a file that no longer exists. +struct Hdf5LeafDeleteObservation { + double sibling_value; // leaf_b read back after al_delete_data("leaf_a") + bool ids_file_exists; // .h5 still on disk + bool occurrence_listed; // master file still links the occurrence +}; + +// Located by name rather than by rebuilding the backend's own on-disk path +// strategy (hdf5_utils.cpp pulseFilePathFactory), which the test has no +// business restating. +inline bool file_exists_under(const std::string& root, const std::string& name) { + namespace fs = std::filesystem; + std::error_code ec; + for (fs::recursive_directory_iterator it(root, ec), end; it != end; + it.increment(ec)) { + if (ec) break; + if (it->path().filename() == name) return true; + } + return false; +} + +Hdf5LeafDeleteObservation hdf5_observe_leaf_delete(al_contract::TempBase& base, + const PulseId& pulse) { const std::string uri = al_contract::build_uri(HDF5_BACKEND, base.str(), pulse); int pctx = -1; EXPECT_EQ(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx).code, 0); @@ -623,37 +650,66 @@ double hdf5_leaf_delete_sibling_survives(al_contract::TempBase& base, EXPECT_EQ(al_delete_data(op, "leaf_a").code, 0); EXPECT_EQ(al_end_action(op).code, 0); - // Reopening a READ on the occurrence may itself fail — HDF5's delete - // removed the whole occurrence's group/link/file, so there may be - // nothing left to open at all. That failure is just as much proof the - // sibling is gone as reading back the sentinel would be. + // Reopening a READ on the occurrence may itself fail — if the delete + // removed the whole occurrence's group/link/file there is nothing left to + // open. That failure is just as much proof the sibling is gone as reading + // back the sentinel would be. int rop = -1; al_status_t open_read = al_begin_global_action(pctx, kIds, "", READ_OP, &rop); - double result = al_contract::kEmptyDouble; + Hdf5LeafDeleteObservation obs{al_contract::kEmptyDouble, false, false}; if (open_read.code == 0) { std::vector shape; std::vector bb; EXPECT_EQ(al_contract::read_data(rop, "leaf_b", 0, &shape, &bb).code, 0); EXPECT_EQ(al_end_action(rop).code, 0); - if (!bb.empty()) result = bb[0]; + if (!bb.empty()) obs.sibling_value = bb[0]; } + + // al_get_occurrences enumerates the master file's links live + // (hdf5_reader.cpp H5Literate over master_file_id), so it reports the link + // whether or not the file it points at still exists — exactly the dangling + // state issue #63 measured. + int* occ = nullptr; + int n = -1; + if (al_get_occurrences(pctx, kIds, &occ, &n).code == 0 && n > 0) + obs.occurrence_listed = true; + free(occ); + + obs.ids_file_exists = file_exists_under(base.str(), std::string(kIds) + ".h5"); + al_close_pulse(pctx, CLOSE_PULSE); - return result; + return obs; } TEST_F(DeleteKnownDefects, DISABLED_Hdf5LeafDeleteLeavesSiblingIntact) { - EXPECT_EQ(hdf5_leaf_delete_sibling_survives(base_, pulse_), 22.0) + EXPECT_EQ(hdf5_observe_leaf_delete(base_, pulse_).sibling_value, 22.0) << "a leaf-granularity delete must not remove sibling fields"; } +TEST_F(DeleteKnownDefects, DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink) { + const Hdf5LeafDeleteObservation obs = hdf5_observe_leaf_delete(base_, pulse_); + EXPECT_TRUE(obs.ids_file_exists) + << "a leaf-granularity delete must not remove the occurrence's pulse file"; + EXPECT_TRUE(obs.occurrence_listed) + << "a leaf-granularity delete must leave the occurrence, and so the " + "master file's link to it, in place"; +} + TEST_F(DeleteKnownDefects, Hdf5LeafDeleteCurrentlyWipesWholeOccurrence) { - EXPECT_EQ(hdf5_leaf_delete_sibling_survives(base_, pulse_), - al_contract::kEmptyDouble) + const Hdf5LeafDeleteObservation obs = hdf5_observe_leaf_delete(base_, pulse_); + EXPECT_EQ(obs.sibling_value, al_contract::kEmptyDouble) << "HDF5 deleteData now honors `path` for leaf granularity — enable " "DeleteKnownDefects.DISABLED_Hdf5LeafDeleteLeavesSiblingIntact " "(src/hdf5/hdf5_writer.cpp deleteData ignores its caller's field)"; + EXPECT_FALSE(obs.ids_file_exists) + << "HDF5 deleteData no longer deletes the whole pulse file for a " + "non-empty path — enable " + "DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink"; + EXPECT_TRUE(obs.occurrence_listed) + << "the master file no longer holds a dangling link after a leaf " + "delete — enable " + "DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink"; } - // --- Memory: no code path implements DATAOBJECT-root delete ---------------- bool memory_root_delete_clears_everything(al_contract::TempBase& base, const PulseId& pulse) { From 088ec5c84e94527334c86e4ea1f0db7b39f1ce9d Mon Sep 17 00:00:00 2001 From: YohannMrg <98689112+yohannmarguier@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:21:40 +0200 Subject: [PATCH 2/4] Make HDF5 al_delete_data honor its `path` argument (issue #63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decision: `path` means what the ABI has always said it means — a non-empty path deletes that node and its subtree only, an empty path addresses the whole DATAOBJECT. Recorded in docs/adr/0001-al-delete-data-path-semantics.md together with the per-backend matrix and why the cheaper alternative (rejecting a non-empty path as a caller error) was rejected: it is not what the header says, not what the other backends do, and it would fail every non-slice `put()` and `delete_data()` through imas-python, whose traversal issues one delete per DD leaf. `HDF5Writer::deleteSubtree` selects the matching datasets out of the IDS group's flat namespace of tensorized names — the mangled path, its `_SHAPE` companion, and anything continuing with `&` (a structure child) or `[]` (an AOS index). The continuation character is what keeps `time` from taking `time_slice[]&x` with it. Buffered writes are flushed before anything is unlinked, since HDF5DataSetHandler::close() frees write buffers without writing them. The group listing is cached across a run of deletes, and invalidated by anything that can add a dataset, so the HLI's leaf-by-leaf traversal is not quadratic in the size of the IDS. `deleteOccurrence` keeps the empty-path behavior and fixes two defects in it: the master file's external link is now removed with the file it points at (it used to be left dangling, so al_get_occurrences kept reporting an occurrence whose file was gone), and a closed-but-present IDS file is deleted instead of being silently reopened and leaked. Tests: DeleteMatrix's HDF5 leaf/structure cells are enabled, the HDF5 xfail pair retired, and the layout-specific consequences the backend-agnostic matrix cannot see are pinned in Hdf5Delete.* — which files and master-file links survive which granularity, same-prefix siblings, an absent path, and a run of successive leaf deletes all taking effect. Verified against the suite, the Debug leg, and ASan/UBSan. (cherry picked from commit c8e49caff006341dc829f343dd9ae86a1124ab77) --- .../adr/0001-al-delete-data-path-semantics.md | 92 +++++ include/al_backend.h | 8 +- include/al_lowlevel.h | 19 +- src/hdf5/hdf5_backend.cpp | 2 +- src/hdf5/hdf5_writer.cpp | 134 +++++-- src/hdf5/hdf5_writer.h | 15 +- tests/contract/TRACEABILITY.md | 15 +- tests/contract/test_structured_data.cpp | 332 ++++++++++++------ 8 files changed, 486 insertions(+), 131 deletions(-) create mode 100644 docs/adr/0001-al-delete-data-path-semantics.md diff --git a/docs/adr/0001-al-delete-data-path-semantics.md b/docs/adr/0001-al-delete-data-path-semantics.md new file mode 100644 index 00000000..49de0621 --- /dev/null +++ b/docs/adr/0001-al-delete-data-path-semantics.md @@ -0,0 +1,92 @@ +--- +status: accepted +--- + +# `al_delete_data`'s `path` selects a node and its subtree, never the occurrence + +`al_delete_data(ctx, path)` has always documented `path` as "the data structure +element to delete (suppress the whole subtree)", but the HDF5 backend dropped +the argument on the floor: `HDF5Backend::deleteData` did not forward it and +`HDF5Writer::deleteData` had no parameter to receive it, so **any** non-empty +path deleted the whole IDS occurrence — the per-IDS pulse file and the master +file's external link to it — and returned success (issue #63). We keep the +documented meaning and implement it: a non-empty `path` deletes that node and +its subtree only, an empty `path` addresses the whole DATAOBJECT, and deleting +an absent path is a successful no-op. + +## Considered options + +**1. Per-path delete is intended (chosen).** Implemented in +`HDF5Writer::deleteSubtree`. An IDS group is a flat namespace of "tensorized" +dataset names — the DD path with `/` → `&`, array-of-structures nodes suffixed +`[]`, one `_SHAPE` companion per non-scalar leaf, one `…[]&AOS_SHAPE` per +dynamic AOS — so the subtree rooted at a DD path is exactly the set of names +that are the mangled path, its `_SHAPE` companion, or that continue with `&` (a +structure child) or `[]` (an AOS element index). The continuation character is +what keeps `time` from taking `time_slice[]&x` with it. + +**2. Occurrence-wide delete is intended: reject a non-empty `path`.** Rejected. +It is the cheaper change, but it is not what the ABI says and not what the other +backends do — and it would break every HDF5 write through the HLI. imas-python's +`delete_children()` walks the DD and issues one `al_delete_data` per leaf +(`imas/backends/imas_core/db_entry_helpers.py`), and `DBEntry.put()` runs that +whole traversal before writing (`db_entry_al.py`), so rejecting non-empty paths +would fail every non-slice `put()` and `DBEntry.delete_data()` on HDF5. Today +that traversal *appears* to work only because the first forwarded delete +destroys the file and the following ones silently do nothing (the first call +closes the IDS group, after which every later call returns early) — and the put +then rewrites everything from scratch. That accident is what hid the defect. + +The third state the code was actually in — argument accepted, ignored, success +reported — is what neither option tolerates, and it is not merely untidy. It is +the mechanism behind a silent data corruption downstream: the +IMAS-Multiversion-DD-Loader refuses to delete an occurrence's DD-version stamp +so a converted pulse cannot hold undateable numbers, but inside a full put the +traversal's first forwarded delete has already taken the whole file, stamp +included, and the rest of the put is written untranslated +([IMAS-Multiversion-DD-Loader#139](https://github.com/yohannmarguier/IMAS-Multiversion-DD-Loader/issues/139)). + +## Per-backend behaviour, as of this ADR + +Checked by reading each `deleteData` and pinned in `tests/contract/` +(`test_structured_data.cpp` `DeleteMatrix` / `Hdf5Delete`, +`test_mdsplus_delete.cpp`, `test_uda_unique_surface.cpp`); the ledger is +`tests/contract/TRACEABILITY.md`. + +| Backend | node | subtree | whole DATAOBJECT (`path == ""`) | +|---|---|---|---| +| HDF5 | ✅ | ✅ | ✅ — deletes `.h5` *and* the master-file link | +| Memory | ✅ | ✅ (AOS node) | ❌ no code path for it — xfail | +| MDSplus | ✅ | ❌ `%TREE-W-NNF` — xfail | ❌ `%TREE-W-NNF` — xfail | +| ASCII | ❌ empty body | ❌ | ❌ | +| Flexbuffers | ❌ empty body | ❌ | ❌ | +| UDA | forwards `path` (remote: no server handler → clean refusal; local/fetch: delegates to the local backend) | " | " | + +So HDF5 was the only backend that *widened* `path`; the rest either honour it or +ignore the call entirely. None of them reports "I cannot honour this path", +which is why the header now says so explicitly rather than leaving an HLI to +discover it. + +`MemoryBackend::deleteData` also carries a dead `ctx->getType() == +CTX_ARRAYSTRUCT_TYPE` branch: the parameter is an `OperationContext*` and +`al_delete_data` `dynamic_cast`s to one, so an arraystruct context can never +reach it. A plain (non-AOS) structure path on Memory is not characterized. + +## Consequences + +- **An emptied occurrence still exists on HDF5.** `DBEntry.delete_data()` walks + the DD and deletes leaf by leaf, so `.h5` and its master-file link + survive with no datasets left in them, and `al_get_occurrences` keeps + reporting the occurrence. That is the honest consequence of `path` meaning + what it says; an HLI that wants the occurrence gone should pass `path == ""`, + which is now the only call that removes it. +- **A delete never shrinks `.h5`.** `H5Ldelete` unlinks; the space is + reclaimed only by repacking the file. +- **The master-file link now follows the file.** A whole-DATAOBJECT delete + removes the external link as well, so the master file no longer advertises an + occurrence whose backing file is gone — the second half of what issue #63 + measured. +- A whole-IDS delete lists the IDS group's link names once per delete rather + than once per leaf (`HDF5Writer::groupMembers`, invalidated by anything that + can add a dataset). Without that cache the HLI's leaf-by-leaf traversal would + be quadratic in the size of the IDS. diff --git a/include/al_backend.h b/include/al_backend.h index aec9860f..314728b0 100644 --- a/include/al_backend.h +++ b/include/al_backend.h @@ -147,8 +147,14 @@ class IMAS_CORE_LIBRARY_API Backend Deletes data. This function deletes some data (can be a signal, a structure, the whole DATAOBJECT) in the database given the passed context. + + An implementation must not widen `path`: a non-empty `path` deletes that + node and its subtree only, and an empty `path` addresses the whole + DATAOBJECT. Deleting a path that holds no data is a no-op, not an error. + See docs/adr/0001-al-delete-data-path-semantics.md. + @param[in] ctx pointer on operation context - @param[in] path path of the data structure element to delete (suppress the whole subtree) + @param[in] path path of the data structure element to delete (suppress the whole subtree); empty means the whole DATAOBJECT @throw BackendException **/ virtual void deleteData(OperationContext *ctx, diff --git a/include/al_lowlevel.h b/include/al_lowlevel.h index baa3ac1e..3e93148a 100644 --- a/include/al_lowlevel.h +++ b/include/al_lowlevel.h @@ -415,8 +415,25 @@ extern "C" Deletes data. This function deletes some data (can be a signal, a structure, the whole DATAOBJECT) in the database given the passed context. + + `path` selects *what* is deleted, and is never widened by the core: + - a non-empty `path` deletes that node and its subtree, and nothing else — + sibling and parent nodes, and the DATAOBJECT itself, survive; + - an empty `path` addresses the whole DATAOBJECT (the occurrence opened by + `ctx`), and is the only way to remove it. + Deleting a path that holds no data is not an error: the call succeeds and + changes nothing. + + Backend support for the three granularities is uneven, and a backend that + cannot honour `path` does not report that it could not: ASCII and + flexbuffers ignore the call entirely, memory implements node and subtree + deletes but not the DATAOBJECT one, MDSplus implements node deletes only. + HDF5 implements all three. The per-backend matrix, and why the argument is + specified this way rather than as an occurrence-wide delete, are recorded + in docs/adr/0001-al-delete-data-path-semantics.md. + @param[in] ctx operation context id (from al_plugin_begin_global_action() or al_plugin_begin_slice_action()) - @param[in] path path of the data structure element to delete (suppress the whole subtree) + @param[in] path path of the data structure element to delete (suppress the whole subtree); empty means the whole DATAOBJECT @result error status [_success if al_status_t.code = 0 or failure if < 0_] */ IMAS_CORE_LIBRARY_API al_status_t al_delete_data(int ctx, diff --git a/src/hdf5/hdf5_backend.cpp b/src/hdf5/hdf5_backend.cpp index cb95f539..2283bd18 100644 --- a/src/hdf5/hdf5_backend.cpp +++ b/src/hdf5/hdf5_backend.cpp @@ -131,7 +131,7 @@ void HDF5Backend::deleteData(OperationContext * ctx, std::string path) { if (file_id == -1) //master file is closed return; - hdf5Writer->deleteData(ctx, this->file_id, opened_IDS_files, files_directory, relative_file_path); + hdf5Writer->deleteData(ctx, path, this->file_id, opened_IDS_files, files_directory, relative_file_path); } void HDF5Backend::beginWriteArraystructAction(ArraystructContext * ctx, int *size) diff --git a/src/hdf5/hdf5_writer.cpp b/src/hdf5/hdf5_writer.cpp index 57d8f702..3782aafc 100644 --- a/src/hdf5/hdf5_writer.cpp +++ b/src/hdf5/hdf5_writer.cpp @@ -14,7 +14,7 @@ using namespace boost::filesystem; HDF5Writer::HDF5Writer(std::string backend_version_) : backend_version(backend_version_), opened_data_sets(), existing_data_sets(), tensorized_paths_per_context(), arrctx_shapes_per_context(), -dynamic_AOS_slices_extension(), homogeneous_time(-1), IDS_group_id(), slice_mode(GLOBAL_OP) +dynamic_AOS_slices_extension(), homogeneous_time(-1), IDS_group_id(), group_members_gid(-1), group_members(), slice_mode(GLOBAL_OP) { //H5Eset_auto2(H5E_DEFAULT, NULL, NULL); } @@ -58,7 +58,56 @@ void HDF5Writer::close_file_handler(std::string external_link_name, std::unorder } } -void HDF5Writer::deleteData(OperationContext * ctx, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path) +// An IDS group is a flat namespace of "tensorized" dataset names: the DD path +// with '/' replaced by '&', every array-of-structures node suffixed "[]", one +// "_SHAPE" companion per non-scalar leaf and one "…[]&AOS_SHAPE" per dynamic +// AOS. So the subtree rooted at a DD path is exactly the set of dataset names +// that are the mangled path itself, its "_SHAPE" companion, or that continue +// with '&' (a structure child) or "[]" (an AOS element index). +// +// The continuation character is what separates a subtree from a same-prefix +// sibling: deleting "time" must not touch "time_slice[]&…". +static bool is_in_subtree(const std::string & dataset_name, const std::string & mangled_path) +{ + if (dataset_name.size() < mangled_path.size() || + dataset_name.compare(0, mangled_path.size(), mangled_path) != 0) + return false; + const std::string continuation = dataset_name.substr(mangled_path.size()); + if (continuation.empty()) // the leaf's own dataset + return true; + if (continuation == "_SHAPE") // its shape companion + return true; + if (continuation[0] == '&') // a child of a structure + return true; + if (continuation.compare(0, 2, "[]") == 0) // "[]&…", "[]&AOS_SHAPE" + return true; + return false; +} + +void HDF5Writer::invalidateGroupMembers() +{ + group_members_gid = -1; + group_members.clear(); +} + +std::vector < std::string > &HDF5Writer::groupMembers(hid_t gid) +{ + if (group_members_gid != gid) { + group_members.clear(); + H5L_iterate_t collect = [](hid_t, const char *name, const H5L_info_t *, void *op_data) -> herr_t { + static_cast < std::vector < std::string > *>(op_data)->push_back(name); + return 0; + }; + if (H5Literate(gid, H5_INDEX_NAME, H5_ITER_NATIVE, NULL, collect, &group_members) < 0) { + group_members.clear(); + throw ALBackendException("HDF5Backend: H5Literate has failed in HDF5Writer::deleteData()", LOG); + } + group_members_gid = gid; + } + return group_members; +} + +void HDF5Writer::deleteData(OperationContext * ctx, const std::string & path, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path) { if (file_id == -1) throw ALBackendException("HDF5Backend: master file not opened in HDF5Writer::deleteData()", LOG); //the master file is assumed to be opened @@ -69,33 +118,73 @@ void HDF5Writer::deleteData(OperationContext * ctx, hid_t file_id, std::unordere if (gid == -1) return; + + // An empty path addresses the whole DATAOBJECT; anything else addresses one + // node and its subtree. See al_lowlevel.h (al_delete_data) and + // docs/adr/0001-al-delete-data-path-semantics.md. + if (path.empty()) + deleteOccurrence(ctx, file_id, opened_IDS_files, files_directory, relative_file_path); + else + deleteSubtree(gid, path); +} + +void HDF5Writer::deleteSubtree(hid_t gid, const std::string & path) +{ + std::string mangled_path = path; + std::replace(mangled_path.begin(), mangled_path.end(), '/', '&'); // character '/' is not supported in datasets names + + // Pending buffered writes have to reach the file before anything is + // unlinked: HDF5DataSetHandler::close() frees its write buffers without + // flushing them, so closing first would silently drop writes made earlier + // in the same WRITE_OP session to datasets this delete does not touch. + if (!opened_data_sets.empty()) + write_buffers(); + close_datasets(); //also clears the existing_data_sets H5Lexists cache + + std::vector < std::string > &members = groupMembers(gid); + std::vector < std::string > doomed; + std::vector < std::string > kept; + for (const std::string & name : members) { + if (is_in_subtree(name, mangled_path)) + doomed.push_back(name); + else + kept.push_back(name); + } + + for (const std::string & name : doomed) { + // H5Ldelete only unlinks: the freed space stays in the file until it is + // repacked, so a delete never shrinks .h5. + if (H5Ldelete(gid, name.c_str(), H5P_DEFAULT) < 0) { + invalidateGroupMembers(); + char error_message[300]; + snprintf(error_message, sizeof(error_message), "Unable to delete HDF5 dataset: %s\n", name.c_str()); + throw ALBackendException(error_message, LOG); + } + } + members = std::move(kept); +} + +void HDF5Writer::deleteOccurrence(OperationContext * ctx, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path) +{ close_datasets(); - close_group(ctx); + close_group(ctx); //also invalidates the cached group listing std::string IDS_link_name = ctx->getDataobjectName(); std::replace(IDS_link_name.begin(), IDS_link_name.end(), '/', '_'); HDF5Utils hdf5_utils; - //Deleting IDS link from master file if (H5Lexists(file_id, IDS_link_name.c_str(), H5P_DEFAULT) > 0) { //the IDS is referenced in the master file - auto got = opened_IDS_files.find(IDS_link_name); - hid_t IDS_file_id = -1; std::string IDSpulseFile = hdf5_utils.getIDSPulseFilePath(files_directory, relative_file_path, IDS_link_name); + auto got = opened_IDS_files.find(IDS_link_name); if (got != opened_IDS_files.end()) { - IDS_file_id = got->second; - if (IDS_file_id < 0) { - if (exists(IDSpulseFile.c_str())) { - hdf5_utils.openIDSFile(ctx, IDSpulseFile, &IDS_file_id, false); - } - } - else { - hdf5_utils.closeIDSFile(IDS_file_id, IDS_link_name); - hdf5_utils.deleteIDSFile(IDSpulseFile); - } + if (got->second >= 0) + hdf5_utils.closeIDSFile(got->second, IDS_link_name); opened_IDS_files[IDS_link_name] = -1; } - else { - if (exists(IDSpulseFile.c_str())) - hdf5_utils.deleteIDSFile(IDSpulseFile); - } + if (exists(IDSpulseFile.c_str())) + hdf5_utils.deleteIDSFile(IDSpulseFile); + //Deleting IDS link from master file: the link has to go with the file it + //points at, or the master keeps advertising an occurrence + //(al_get_occurrences enumerates these links) whose backing file is gone. + hdf5_utils.removeLinkFromMasterPulseFile(file_id, IDS_link_name); } } @@ -171,7 +260,7 @@ void HDF5Writer::create_IDS_group(OperationContext * ctx, hid_t file_id, std::un throw ALBackendException(error_message, LOG); } } - close_group(ctx); + close_group(ctx); //also invalidates the cached group listing hid_t loc_id = hdf5_utils.createOrOpenHDF5Group(ctx->getDataobjectName().c_str(), IDS_file_id); if (! (loc_id >= 0)) throw ALBackendException("HDF5Backend: unexpected value for loc_id in HDF5Writer::create_IDS_group()", LOG); @@ -194,6 +283,7 @@ void HDF5Writer::close_datasets() void HDF5Writer::close_group(OperationContext *ctx) { + invalidateGroupMembers(); hid_t gid = -1; auto got = IDS_group_id.find(ctx); if (got != IDS_group_id.end()) { @@ -279,6 +369,7 @@ int HDF5Writer::readTimedAOSShape(hid_t loc_id, std::string &tensorized_path, co void HDF5Writer::beginWriteArraystructAction(ArraystructContext * ctx, int *size) { + invalidateGroupMembers(); //may add the AOS_SHAPE dataset to the group HDF5Utils hdf5_utils; OperationContext *opctx = ctx->getOperationContext(); hid_t gid = -1; @@ -342,6 +433,7 @@ ArraystructContext* HDF5Writer::getDynamicAOS(Context * ctx) { void HDF5Writer::write_ND_Data(Context * ctx, std::string & att_name, std::string & timebasename, int datatype, int dim, int *size, void *data) { + invalidateGroupMembers(); //may add the value and _SHAPE datasets to the group std::string & dataset_name = att_name; std::replace(dataset_name.begin(), dataset_name.end(), '/', '&'); // character '/' is not supported in datasets names std::replace(timebasename.begin(), timebasename.end(), '/', '&'); diff --git a/src/hdf5/hdf5_writer.h b/src/hdf5/hdf5_writer.h index 70caf417..71014d35 100644 --- a/src/hdf5/hdf5_writer.h +++ b/src/hdf5/hdf5_writer.h @@ -21,6 +21,15 @@ class HDF5Writer { int homogeneous_time; std::unordered_map < OperationContext *, hid_t> IDS_group_id; + + // Link names of one IDS group, cached for the duration of a run of + // deleteData() calls. Reset by invalidateGroupMembers() on anything that + // can add a dataset to a group or swap the group itself, so an over-eager + // reset is always the safe direction. Without it a whole-IDS delete (the + // HLI issues one al_delete_data per DD leaf) would re-list the group once + // per leaf. + hid_t group_members_gid; + std::vector < std::string > group_members; int slice_mode; @@ -30,6 +39,10 @@ class HDF5Writer { int readTimedAOSShape(Context * ctx, hid_t loc_id, const std::vector < int > ¤t_arrctx_indices); int readTimedAOSShape(hid_t loc_id, std::string &tensorized_path, const std::vector < int > ¤t_arrctx_indices, uri::Uri uri); int getDynamic_AOS_slices_extension(Context *ctx); + void deleteSubtree(hid_t gid, const std::string & path); + void deleteOccurrence(OperationContext * ctx, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path); + std::vector < std::string > &groupMembers(hid_t gid); + void invalidateGroupMembers(); int getDynamic_slices_extension(Context *ctx, int timed_AOS_index, int time_vector_length); ArraystructContext* getDynamicAOS(Context * ctx); @@ -44,7 +57,7 @@ class HDF5Writer { static size_t write_chunk_cache_size; virtual void closePulse(DataEntryContext * ctx, int mode, hid_t *file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, int files_path_strategy, std::string & files_directory, std::string & relative_file_path); - virtual void deleteData(OperationContext * ctx, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path); + virtual void deleteData(OperationContext * ctx, const std::string & path, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path); virtual void write_ND_Data(Context * ctx, std::string & att_name, std::string & timebasename, int datatype, int dim, int *size, void *data); virtual void beginWriteArraystructAction(ArraystructContext * ctx, int *size); diff --git a/tests/contract/TRACEABILITY.md b/tests/contract/TRACEABILITY.md index 23be3fdd..7d01a395 100644 --- a/tests/contract/TRACEABILITY.md +++ b/tests/contract/TRACEABILITY.md @@ -57,10 +57,11 @@ carry a `/` (or `/__r`) suffix per instance. The suite covers the **data-path, introspection, structured-data surface, plugin-management surface, and Plugin-author audience** end to end and pins -the defects those issues surfaced (twenty xfails, each with a paired +the defects those issues surfaced (nineteen xfails, each with a paired current-behavior tripwire — issue #8 resolved every open ownership/coverage -question by test but found no new genuine defects, so the xfail count is -unchanged). Issue #8 (`6-ownership-sweep.md`) closed every remaining row: +question by test but found no new genuine defects, so it left the xfail count +unchanged; issue #63 retired one by fixing it, HDF5's occurrence-wide +`al_delete_data` widening). Issue #8 (`6-ownership-sweep.md`) closed every remaining row: - **Ownership**: `al_context_info`, `al_get_occurrences`'s `*occurrences_list`, and `al_list_filled_paths`'s list+strings were already @@ -127,8 +128,8 @@ genuine defect (expected-fail). | ↳ CHAR > 2-D refused (documented "not implemented") on HDF5·ASCII | :244-263 / D4 | — | ✓ | `…/{HDF5,ASCII}_CHAR_r{3..7}` (paired-negative) | covered | | ↳ CHAR scalar (dim 0) must round-trip — **HDF5 crashes** | :244-263 | — | ✓ | `RoundTripKnownDefects.DISABLED_Hdf5CharScalarRoundTrips` + tripwire `RoundTripKnownDefectsDeath.Hdf5CharScalarCurrentlyCrashes` | **xfail** | | ↳ numeric at MAXDIM (rank 7) must round-trip — **ASCII corrupts (DOUBLE/COMPLEX) / aborts (INTEGER)** | :244-263 | — | ✓ | `RoundTripKnownDefects.DISABLED_Ascii{Integer,Double,Complex}MaxdimRoundTrips` + tripwires `RoundTripKnownDefectsDeath.AsciiIntegerMaxdimCurrentlyAborts`, `RoundTripKnownDefectsDeath.Ascii{Double,Complex}MaxdimCurrentlyCorrupts` (death-test children: corruption *or* crash keeps them green — the overrun's manifestation is machine-dependent) | **xfail** | -| `al_delete_data` at signal / structure / DATAOBJECT-root granularity | :265-272 | ✓ (Memory) | ✓ (HDF5) | Support is disjoint per backend, none implement all three (see xfail rows below): `Backends/DeleteMatrix.{LeafDeleteRemovesJustTheLeaf,StructureDeleteRemovesWholeSubtree}/Memory` (leaf + structure genuinely work only on Memory), `…/RootDeleteRemovesWholeOccurrence/HDF5` (root genuinely works only on HDF5, which ignores `path` and always removes the whole occurrence). MDSplus: not joined to this fixture — terminal gap, see Part 4 (issue #18). UDA: remote delete is cleanly refused (the reference plugin has no `deleteData` handler); remote write over the same path falsely reports success instead of the same refusal — xfail, see Part 5 | covered | -| ↳ HDF5 ignores `path` for leaf/structure delete — **any delete call removes the whole occurrence**, not just the named field | :265-272; src/hdf5/hdf5_writer.cpp deleteData (no path parameter at all) | — | ✓ | One tripwire covers both defective granularities, since there is no leaf-vs-structure branch to differ (`HDF5Writer::deleteData` never receives a path at all): `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteLeavesSiblingIntact` + `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink` + tripwire `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence`, which pins all three observable consequences (sibling gone, `.h5` deleted, master file left holding a dangling external link — issue #63); matrix cells skip via `Backends/DeleteMatrix.{Leaf,Structure}*/HDF5` | **xfail** | +| `al_delete_data` at signal / structure / DATAOBJECT-root granularity | :265-272 | ✓ (Memory) | ✓ (HDF5) | Support is still uneven per backend (see xfail rows below), but HDF5 now implements all three: `Backends/DeleteMatrix.{LeafDeleteRemovesJustTheLeaf,StructureDeleteRemovesWholeSubtree}/{HDF5,Memory}`, `…/RootDeleteRemovesWholeOccurrence/HDF5`. HDF5 used to ignore `path` and widen every call to the whole occurrence — fixed in issue #63, whose HDF5-layout-specific consequences (which files and master-file links survive which granularity, and that a same-prefix sibling is not collateral) are pinned separately in `Hdf5Delete.*`, since they are invisible to this backend-agnostic matrix. The decided meaning of `path` and the per-backend matrix are recorded in `docs/adr/0001-al-delete-data-path-semantics.md`. MDSplus: not joined to this fixture — terminal gap, see Part 4 (issue #18). UDA: remote delete is cleanly refused (the reference plugin has no `deleteData` handler); remote write over the same path falsely reports success instead of the same refusal — xfail, see Part 5 | covered | +| ↳ HDF5 honors `path`: a non-empty path deletes that node and its subtree only, an empty path the whole DATAOBJECT (issue #63 — it previously widened every call to the occurrence and left `master.h5` holding a dangling external link) | :265-272; src/hdf5/hdf5_writer.cpp `deleteData` / `deleteSubtree` / `deleteOccurrence` | — | ✓ | `Hdf5Delete.{LeafDeleteKeepsOccurrenceFileAndMasterLink,RootDeleteRemovesOccurrenceFileAndMasterLink,SamePrefixSiblingsSurviveASubtreeDelete,DeletingAnAbsentPathChangesNothing,ManySuccessiveLeafDeletesAllTakeEffect}` — the last one covers the HLI's actual usage (imas-python `delete_children()` issues one delete per DD leaf), which the old code silently no-op'd after the first call closed the IDS group | covered | | ↳ Memory has no code path for DATAOBJECT-root delete — **a root-granularity delete is silently a no-op** | :265-272; src/memory_backend.cpp deleteData (no root/occurrence case) | ✓ | — | `DeleteKnownDefects.DISABLED_MemoryRootDeleteClearsWholeIds` + tripwire `DeleteKnownDefects.MemoryRootDeleteCurrentlyDoesNothing`; matrix cell skips via `Backends/DeleteMatrix.RootDeleteRemovesWholeOccurrence/Memory` | **xfail** | | ↳ ASCII's `deleteData` is a literal no-op (empty body) at every granularity; Flexbuffers' is too but is not independently ABI-observable (it already refuses every in-session read regardless of delete, so a read-based oracle can't distinguish "delete did nothing" from "this backend never round-trips reads") | :265-272; src/ascii_backend.cpp:648-652, src/flexbuffers_backend.cpp:387-389 | ✓ | ✓ | One tripwire covers every granularity on ASCII, since an empty function body has no path-dependent branch to differ: `DeleteKnownDefects.DISABLED_AsciiDeleteRemovesLeaf` + tripwire `DeleteKnownDefects.AsciiDeleteIsCurrentlyANoOp`; matrix cells skip via `Backends/DeleteMatrix.*/ASCII` and `…/Flexbuffers` | **xfail** | | `al_iterate_over_arraystruct` advances the AOS cursor | :274-282 | ✓ (Memory) | ✓ (HDF5) | Exercised as an asserted dependency of every AOS write/read in `Backends/AosMatrix.*` (top-level and nested, multi-step) | covered (indirect) | @@ -225,7 +226,7 @@ and points at the same test. | **B** `openPulse` / `closePulse` | :542-552 | ✓ (Memory) | ✓ (HDF5, ASCII) | via `al_begin_dataentry_action` / `al_close_pulse` in all suites | covered (indirect) | | **C** `beginAction` / `endAction` | :558-574 | ✓ (Memory) | ✓ (HDF5, ASCII) | via `al_begin_global_action` / `al_end_action` | covered (indirect) | | **C** `writeData` / `readData` (0=not-found vs 1=success inner convention) | :576-589 | ✓ (Memory) | ✓ (HDF5, ASCII) | via `al_write_data` / `al_read_data` — `RoundTripMatrix` (+ the CHAR-scalar / ASCII-maxdim xfails above). MDSplus: see Part 4's real-DD-path breadth matrix (issue #15) instead of `RoundTripMatrix`. UDA: same — see Part 5's real-DD-path breadth matrix instead of `RoundTripMatrix` | covered | -| **C** `deleteData` | :591-599 | ✓ (Memory) | ✓ (HDF5) | via `al_delete_data` — see Cluster 2 row above and its three xfail rows. MDSplus: terminal gap — see Part 4 (issue #18). UDA: remote delete cleanly refused, remote write falsely reports success — xfail, see Part 5 | covered (indirect) | +| **C** `deleteData` | :591-599 | ✓ (Memory) | ✓ (HDF5) | via `al_delete_data` — see Cluster 2 row above and its two remaining xfail rows (Memory root, ASCII/Flexbuffers no-op); HDF5 is covered at all three granularities since issue #63. MDSplus: terminal gap — see Part 4 (issue #18). UDA: remote delete cleanly refused, remote write falsely reports success — xfail, see Part 5 | covered (indirect) | | **C** `beginArraystructAction` | :601-606 | ✓ (Memory) | ✓ (HDF5) | via `al_begin_arraystruct_action` — see Cluster 2 row above. MDSplus: joined (divergence) plus its own unique struct-aware slice/segment-write surface — see Part 4. UDA: traversal mechanism covered, a dynamic leaf nested inside a struct_array returns the wrong value — xfail, see Part 5 | covered (indirect) | | **D** `get_occurrences` | :612-616 | — | ✓ | via `al_get_occurrences` — `Occurrences.*`. MDSplus: see Part 4. UDA: see Part 5 | covered (indirect) | | **D** `list_filled_paths` (per-backend: HDF5 real, others throw) | :618-640 | ✓ (Memory) | ✓ (HDF5) | via `al_list_filled_paths` — `CapabilityMatrix.ListFilledPathsPositiveOrRefused` pins both the HDF5 impl and the unconditional throw on Memory·ASCII·Flexbuffers. MDSplus: throws unconditionally too, same as Memory·ASCII·Flexbuffers — see Part 4. UDA: real over remote mode, restricted to `backend=hdf5` — see Part 5 | covered | @@ -275,8 +276,6 @@ whoever fixed it to enable the paired `DISABLED_` correct-contract test: | `ERASE_PULSE` == `CLOSE_PULSE` on Memory | `…DISABLED_MemoryEraseMakesPulseUnopenable` | `ErasePulseKnownDefects.MemoryEraseCurrentlyLeavesPulseOpenable` | | `al_get_backendID` accepts a non-pulse context via unchecked `static_cast` | `GetBackendIdKnownDefects.DISABLED_WrongContextTypeReturnsError` | `GetBackendIdKnownDefects.WrongContextTypeCurrentlySucceedsViaUnsafeCast` | | ASCII AOS read always reports size 0 | `AosKnownDefects.DISABLED_AsciiAosReadReportsWrittenSize` | `AosKnownDefects.AsciiAosReadCurrentlyReportsZero` | -| HDF5 leaf/structure delete wipes the whole occurrence | `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteLeavesSiblingIntact` | `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence` | -| HDF5 leaf delete deletes `.h5` and leaves a dangling master-file link (issue #63) | `DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink` | `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence` | | Memory root delete is a no-op | `DeleteKnownDefects.DISABLED_MemoryRootDeleteClearsWholeIds` | `DeleteKnownDefects.MemoryRootDeleteCurrentlyDoesNothing` | | ASCII delete is a no-op at every granularity | `DeleteKnownDefects.DISABLED_AsciiDeleteRemovesLeaf` | `DeleteKnownDefects.AsciiDeleteIsCurrentlyANoOp` | diff --git a/tests/contract/test_structured_data.cpp b/tests/contract/test_structured_data.cpp index b2e2d63a..4cbd2bab 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -430,14 +430,13 @@ INSTANTIATE_TEST_SUITE_P( // =========================================================================== // The three granularities land on wildly different, non-overlapping support // across the always-on tier (verified by reading each backend's deleteData): -// - HDF5: ignores `path` entirely — every call deletes the whole occurrence. -// So *root* is the only granularity that matches the documented contract; -// leaf/structure are genuine defects (a "leaf" delete takes everything -// else with it). One tripwire (leaf) covers both defective granularities: -// `HDF5Writer::deleteData` (src/hdf5/hdf5_writer.cpp) takes no path -// parameter at all, so there is no leaf-vs-structure branch whose -// behavior could differ — a structure-shaped call hits the exact same -// unconditional occurrence wipe. +// - HDF5: all three work. `path` used to be dropped on the floor, so every +// call widened to the whole occurrence (issue #63); it now selects the +// matching datasets out of the IDS group's flat namespace, and only an +// empty path removes the occurrence. The HDF5-specific consequences — +// which files and master-file links survive which granularity, and that a +// same-prefix sibling is not collateral — are pinned in Hdf5Delete.* below, +// since they are not observable through this backend-agnostic matrix. // - Memory: the only backend that honors `path` — leaf and structure (an // AOS field) both work for real; there is no code path that special-cases // "delete the whole IDS", so root is the defect here. @@ -462,8 +461,8 @@ struct DeleteBackendCase { inline void PrintTo(const DeleteBackendCase& b, std::ostream* os) { *os << b.name; } const DeleteBackendCase kDeleteBackends[] = { - {HDF5_BACKEND, "HDF5", /*on_disk=*/true, /*leaf_ok=*/false, - /*structure_ok=*/false, /*root_ok=*/true}, + {HDF5_BACKEND, "HDF5", /*on_disk=*/true, /*leaf_ok=*/true, + /*structure_ok=*/true, /*root_ok=*/true}, {MEMORY_BACKEND, "Memory", /*on_disk=*/false, /*leaf_ok=*/true, /*structure_ok=*/true, /*root_ok=*/false}, {ASCII_BACKEND, "ASCII", /*on_disk=*/true, /*leaf_ok=*/false, @@ -608,16 +607,106 @@ class DeleteKnownDefects : public ::testing::Test { } }; -const BackendCase kHdf5{HDF5_BACKEND, "HDF5", /*on_disk=*/true}; const BackendCase kAscii{ASCII_BACKEND, "ASCII", /*on_disk=*/true}; -// --- HDF5: leaf delete ignores `path`, wipes the whole occurrence ---------- +// --- Memory: no code path implements DATAOBJECT-root delete ---------------- +bool memory_root_delete_clears_everything(al_contract::TempBase& base, + const PulseId& pulse) { + const std::string uri = al_contract::build_uri(MEMORY_BACKEND, base.str(), pulse); + int pctx = -1; + EXPECT_EQ(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx).code, 0); + int op = -1; + EXPECT_EQ(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op).code, 0); + EXPECT_EQ(al_contract::write_data(op, "leaf_a", {}, {11.0}).code, 0); + EXPECT_EQ(al_contract::write_data(op, "leaf_b", {}, {22.0}).code, 0); + EXPECT_EQ(al_delete_data(op, "").code, 0); + EXPECT_EQ(al_end_action(op).code, 0); + int rop = -1; + EXPECT_EQ(al_begin_global_action(pctx, kIds, "", READ_OP, &rop).code, 0); + std::vector shape; + std::vector a, bb; + EXPECT_EQ(al_contract::read_data(rop, "leaf_a", 0, &shape, &a).code, 0); + EXPECT_EQ(al_contract::read_data(rop, "leaf_b", 0, &shape, &bb).code, 0); + EXPECT_EQ(al_end_action(rop).code, 0); + al_close_pulse(pctx, CLOSE_PULSE); + return !a.empty() && !bb.empty() && a[0] == al_contract::kEmptyDouble && + bb[0] == al_contract::kEmptyDouble; +} + +TEST_F(DeleteKnownDefects, DISABLED_MemoryRootDeleteClearsWholeIds) { + EXPECT_TRUE(memory_root_delete_clears_everything(base_, pulse_)) + << "a DATAOBJECT-root delete must clear every field of the IDS"; +} + +TEST_F(DeleteKnownDefects, MemoryRootDeleteCurrentlyDoesNothing) { + EXPECT_FALSE(memory_root_delete_clears_everything(base_, pulse_)) + << "MemoryBackend::deleteData now supports root granularity — enable " + "DeleteKnownDefects.DISABLED_MemoryRootDeleteClearsWholeIds " + "(memory_backend.cpp deleteData has no root/occurrence case)"; +} + +// --- ASCII: deleteData is a literal no-op ---------------------------------- +// Flexbuffers is deliberately NOT covered by this pair: it refuses every +// in-session read regardless of delete (RoundTripMatrix classifies the whole +// backend as a must-refuse column — "write accepted, read refused, every +// datatype x shape"), so a read-based oracle can't distinguish "delete did +// nothing" from "this backend never round-trips reads at all." Its +// deleteData is confirmed a no-op by reading the source directly +// (flexbuffers_backend.cpp: empty NOOP body) — noted in the file-header +// comment above, not re-asserted here since it isn't ABI-observable. +bool delete_is_a_noop(const BackendCase& b, al_contract::TempBase& base, + const PulseId& pulse) { + const std::string uri = al_contract::build_uri(b.id, base.str(), pulse); + int pctx = -1; + EXPECT_EQ(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx).code, 0); + int op = -1; + EXPECT_EQ(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op).code, 0); + EXPECT_EQ(al_contract::write_data(op, "leaf_a", {}, {11.0}).code, 0); + EXPECT_EQ(al_delete_data(op, "leaf_a").code, 0); + EXPECT_EQ(al_end_action(op).code, 0); + int rop = -1; + EXPECT_EQ(al_begin_global_action(pctx, kIds, "", READ_OP, &rop).code, 0); + std::vector shape; + std::vector a; + EXPECT_EQ(al_contract::read_data(rop, "leaf_a", 0, &shape, &a).code, 0); + EXPECT_EQ(al_end_action(rop).code, 0); + al_close_pulse(pctx, CLOSE_PULSE); + return !a.empty() && a[0] == 11.0; // still there => delete did nothing +} + +TEST_F(DeleteKnownDefects, DISABLED_AsciiDeleteRemovesLeaf) { + EXPECT_FALSE(delete_is_a_noop(kAscii, base_, pulse_)) + << "al_delete_data must actually remove data in the ASCII backend"; +} +TEST_F(DeleteKnownDefects, AsciiDeleteIsCurrentlyANoOp) { + EXPECT_TRUE(delete_is_a_noop(kAscii, base_, pulse_)) + << "AsciiBackend::deleteData now has an effect — enable " + "DeleteKnownDefects.DISABLED_AsciiDeleteRemovesLeaf " + "(ascii_backend.cpp deleteData has an empty body)"; +} + + +// =========================================================================== +// HDF5-specific al_delete_data consequences (issue #63). +// =========================================================================== +// DeleteMatrix above asks the backend-agnostic question ("is the named node +// gone, is its sibling still there"). Three things it cannot see matter on +// HDF5, because they are properties of its two-level file layout rather than +// of the C ABI: +// +// 1. Which *files* survive. A non-empty path must leave .h5 alone; only +// an empty path (the whole DATAOBJECT) removes it. Before issue #63 every +// delete removed it. +// 2. Whether master.h5 still links the occurrence. The link has to follow the +// file: leaving it behind advertises an occurrence — al_get_occurrences +// enumerates exactly these links — whose backing file is gone. +// 3. Same-prefix siblings. The IDS group is a flat namespace of mangled path +// names ('/' -> '&', AOS nodes suffixed "[]"), so "time" and +// "time_slice[]&x" are neighbours in one namespace and a prefix match +// alone would take the second with the first. // One non-empty-path delete of a single leaf, observed on three axes: the // sibling leaf, the occurrence's own pulse file, and the external link the -// master file holds for that occurrence. The last two are what issue #63 -// measured, and they are why the widening is not merely untidy — the call -// reports success while removing the whole occurrence, and leaves master.h5 -// pointing at a file that no longer exists. +// master file holds for that occurrence. struct Hdf5LeafDeleteObservation { double sibling_value; // leaf_b read back after al_delete_data("leaf_a") bool ids_file_exists; // .h5 still on disk @@ -681,111 +770,158 @@ Hdf5LeafDeleteObservation hdf5_observe_leaf_delete(al_contract::TempBase& base, return obs; } -TEST_F(DeleteKnownDefects, DISABLED_Hdf5LeafDeleteLeavesSiblingIntact) { - EXPECT_EQ(hdf5_observe_leaf_delete(base_, pulse_).sibling_value, 22.0) - << "a leaf-granularity delete must not remove sibling fields"; -} +class Hdf5Delete : public ::testing::Test { +protected: + al_contract::TempBase base_; + PulseId pulse_{/*database=*/"test", /*version=*/"3", /*pulse=*/12, /*run=*/0}; + + int open() { + base_.make_legacy_tree(pulse_); + const std::string uri = al_contract::build_uri(HDF5_BACKEND, base_.str(), pulse_); + int pctx = -1; + EXPECT_EQ(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx).code, 0); + return pctx; + } + + bool ids_file_exists() const { + return file_exists_under(base_.str(), std::string(kIds) + ".h5"); + } + + // True iff master.h5 still holds an external link for the occurrence. + bool occurrence_listed(int pctx) const { + int* occ = nullptr; + int n = -1; + const bool ok = al_get_occurrences(pctx, kIds, &occ, &n).code == 0 && n > 0; + free(occ); + return ok; + } +}; -TEST_F(DeleteKnownDefects, DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink) { +TEST_F(Hdf5Delete, LeafDeleteKeepsOccurrenceFileAndMasterLink) { const Hdf5LeafDeleteObservation obs = hdf5_observe_leaf_delete(base_, pulse_); + EXPECT_EQ(obs.sibling_value, 22.0) + << "a leaf-granularity delete must not remove sibling fields"; EXPECT_TRUE(obs.ids_file_exists) << "a leaf-granularity delete must not remove the occurrence's pulse file"; EXPECT_TRUE(obs.occurrence_listed) - << "a leaf-granularity delete must leave the occurrence, and so the " - "master file's link to it, in place"; + << "the occurrence, and so the master file's link to it, must survive a " + "leaf-granularity delete"; } -TEST_F(DeleteKnownDefects, Hdf5LeafDeleteCurrentlyWipesWholeOccurrence) { - const Hdf5LeafDeleteObservation obs = hdf5_observe_leaf_delete(base_, pulse_); - EXPECT_EQ(obs.sibling_value, al_contract::kEmptyDouble) - << "HDF5 deleteData now honors `path` for leaf granularity — enable " - "DeleteKnownDefects.DISABLED_Hdf5LeafDeleteLeavesSiblingIntact " - "(src/hdf5/hdf5_writer.cpp deleteData ignores its caller's field)"; - EXPECT_FALSE(obs.ids_file_exists) - << "HDF5 deleteData no longer deletes the whole pulse file for a " - "non-empty path — enable " - "DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink"; - EXPECT_TRUE(obs.occurrence_listed) - << "the master file no longer holds a dangling link after a leaf " - "delete — enable " - "DeleteKnownDefects.DISABLED_Hdf5LeafDeleteKeepsOccurrenceFileAndLink"; +TEST_F(Hdf5Delete, RootDeleteRemovesOccurrenceFileAndMasterLink) { + const int pctx = open(); + int op = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op)); + AL_EXPECT_OK(al_contract::write_data(op, "leaf_a", {}, {11.0})); + AL_EXPECT_OK(al_delete_data(op, "")); + AL_ASSERT_OK(al_end_action(op)); + + EXPECT_FALSE(ids_file_exists()) + << "an empty path addresses the whole DATAOBJECT: .h5 must go"; + EXPECT_FALSE(occurrence_listed(pctx)) + << "the master file must not keep an external link to a pulse file that " + "was just deleted (issue #63)"; + + al_close_pulse(pctx, CLOSE_PULSE); } -// --- Memory: no code path implements DATAOBJECT-root delete ---------------- -bool memory_root_delete_clears_everything(al_contract::TempBase& base, - const PulseId& pulse) { - const std::string uri = al_contract::build_uri(MEMORY_BACKEND, base.str(), pulse); - int pctx = -1; - EXPECT_EQ(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx).code, 0); + +// The flat-namespace hazard, in both continuation forms: "time" vs the AOS +// "time_slice[]&…" ("[]"), and "code" vs the leaf "code_name" (neither '&' nor +// "[]", so not a child at all). +TEST_F(Hdf5Delete, SamePrefixSiblingsSurviveASubtreeDelete) { + const int pctx = open(); int op = -1; - EXPECT_EQ(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op).code, 0); - EXPECT_EQ(al_contract::write_data(op, "leaf_a", {}, {11.0}).code, 0); - EXPECT_EQ(al_contract::write_data(op, "leaf_b", {}, {22.0}).code, 0); - EXPECT_EQ(al_delete_data(op, "").code, 0); - EXPECT_EQ(al_end_action(op).code, 0); + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op)); + AL_EXPECT_OK(al_contract::write_data(op, "time", {}, {7.0})); + AL_EXPECT_OK(al_contract::write_data(op, "code_name", {}, {8.0})); + AL_EXPECT_OK(al_contract::write_data(op, "code/version", {}, {9.0})); + int size = 1; + int aos = -1; + AL_ASSERT_OK(al_begin_arraystruct_action(op, "time_slice", "", &size, &aos)); + AL_EXPECT_OK(al_contract::write_data(aos, "x", {}, {5.0})); + AL_ASSERT_OK(al_end_action(aos)); + AL_EXPECT_OK(al_delete_data(op, "time")); + AL_EXPECT_OK(al_delete_data(op, "code")); + AL_ASSERT_OK(al_end_action(op)); + int rop = -1; - EXPECT_EQ(al_begin_global_action(pctx, kIds, "", READ_OP, &rop).code, 0); + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", READ_OP, &rop)); std::vector shape; - std::vector a, bb; - EXPECT_EQ(al_contract::read_data(rop, "leaf_a", 0, &shape, &a).code, 0); - EXPECT_EQ(al_contract::read_data(rop, "leaf_b", 0, &shape, &bb).code, 0); - EXPECT_EQ(al_end_action(rop).code, 0); - al_close_pulse(pctx, CLOSE_PULSE); - return !a.empty() && !bb.empty() && a[0] == al_contract::kEmptyDouble && - bb[0] == al_contract::kEmptyDouble; -} + std::vector t, code_name, code_version, x; + AL_EXPECT_OK(al_contract::read_data(rop, "time", 0, &shape, &t)); + AL_EXPECT_OK(al_contract::read_data(rop, "code_name", 0, &shape, &code_name)); + AL_EXPECT_OK(al_contract::read_data(rop, "code/version", 0, &shape, &code_version)); + int rsize = -1; + int raos = -1; + AL_ASSERT_OK(al_begin_arraystruct_action(rop, "time_slice", "", &rsize, &raos)); + ASSERT_EQ(rsize, 1) << "the time_slice AOS must survive deleting the leaf \"time\""; + AL_EXPECT_OK(al_contract::read_data(raos, "x", 0, &shape, &x)); + AL_ASSERT_OK(al_end_action(raos)); + AL_ASSERT_OK(al_end_action(rop)); -TEST_F(DeleteKnownDefects, DISABLED_MemoryRootDeleteClearsWholeIds) { - EXPECT_TRUE(memory_root_delete_clears_everything(base_, pulse_)) - << "a DATAOBJECT-root delete must clear every field of the IDS"; -} + EXPECT_EQ(t.at(0), al_contract::kEmptyDouble) << "the named leaf must be gone"; + EXPECT_EQ(code_version.at(0), al_contract::kEmptyDouble) + << "a structure delete must take its children"; + EXPECT_EQ(x.at(0), 5.0) << "\"time_slice[]&x\" is not part of the \"time\" subtree"; + EXPECT_EQ(code_name.at(0), 8.0) + << "\"code_name\" is not part of the \"code\" subtree"; -TEST_F(DeleteKnownDefects, MemoryRootDeleteCurrentlyDoesNothing) { - EXPECT_FALSE(memory_root_delete_clears_everything(base_, pulse_)) - << "MemoryBackend::deleteData now supports root granularity — enable " - "DeleteKnownDefects.DISABLED_MemoryRootDeleteClearsWholeIds " - "(memory_backend.cpp deleteData has no root/occurrence case)"; + al_close_pulse(pctx, CLOSE_PULSE); } -// --- ASCII: deleteData is a literal no-op ---------------------------------- -// Flexbuffers is deliberately NOT covered by this pair: it refuses every -// in-session read regardless of delete (RoundTripMatrix classifies the whole -// backend as a must-refuse column — "write accepted, read refused, every -// datatype x shape"), so a read-based oracle can't distinguish "delete did -// nothing" from "this backend never round-trips reads at all." Its -// deleteData is confirmed a no-op by reading the source directly -// (flexbuffers_backend.cpp: empty NOOP body) — noted in the file-header -// comment above, not re-asserted here since it isn't ABI-observable. -bool delete_is_a_noop(const BackendCase& b, al_contract::TempBase& base, - const PulseId& pulse) { - const std::string uri = al_contract::build_uri(b.id, base.str(), pulse); - int pctx = -1; - EXPECT_EQ(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx).code, 0); +TEST_F(Hdf5Delete, DeletingAnAbsentPathChangesNothing) { + const int pctx = open(); int op = -1; - EXPECT_EQ(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op).code, 0); - EXPECT_EQ(al_contract::write_data(op, "leaf_a", {}, {11.0}).code, 0); - EXPECT_EQ(al_delete_data(op, "leaf_a").code, 0); - EXPECT_EQ(al_end_action(op).code, 0); + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op)); + AL_EXPECT_OK(al_contract::write_data(op, "leaf_a", {}, {11.0})); + AL_EXPECT_OK(al_delete_data(op, "never_written")) + << "deleting what is not there is not an error"; + AL_ASSERT_OK(al_end_action(op)); + int rop = -1; - EXPECT_EQ(al_begin_global_action(pctx, kIds, "", READ_OP, &rop).code, 0); + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", READ_OP, &rop)); std::vector shape; std::vector a; - EXPECT_EQ(al_contract::read_data(rop, "leaf_a", 0, &shape, &a).code, 0); - EXPECT_EQ(al_end_action(rop).code, 0); + AL_EXPECT_OK(al_contract::read_data(rop, "leaf_a", 0, &shape, &a)); + AL_ASSERT_OK(al_end_action(rop)); + EXPECT_EQ(a.at(0), 11.0) << "an absent-path delete must not touch anything else"; + al_close_pulse(pctx, CLOSE_PULSE); - return !a.empty() && a[0] == 11.0; // still there => delete did nothing } -TEST_F(DeleteKnownDefects, DISABLED_AsciiDeleteRemovesLeaf) { - EXPECT_FALSE(delete_is_a_noop(kAscii, base_, pulse_)) - << "al_delete_data must actually remove data in the ASCII backend"; -} -TEST_F(DeleteKnownDefects, AsciiDeleteIsCurrentlyANoOp) { - EXPECT_TRUE(delete_is_a_noop(kAscii, base_, pulse_)) - << "AsciiBackend::deleteData now has an effect — enable " - "DeleteKnownDefects.DISABLED_AsciiDeleteRemovesLeaf " - "(ascii_backend.cpp deleteData has an empty body)"; -} +// The whole-IDS delete the HLI actually issues: imas-python's delete_children() +// walks the DD and sends one al_delete_data per leaf, so the *second* call has +// to still find a live group. It did not before issue #63: the first call +// closed the group, and every later one returned early having done nothing. +TEST_F(Hdf5Delete, ManySuccessiveLeafDeletesAllTakeEffect) { + const int pctx = open(); + int op = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op)); + for (const char* leaf : {"leaf_a", "leaf_b", "leaf_c"}) + AL_EXPECT_OK(al_contract::write_data(op, leaf, {}, {1.0})); + AL_ASSERT_OK(al_end_action(op)); + + int dop = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", WRITE_OP, &dop)); + for (const char* leaf : {"leaf_a", "leaf_b"}) + AL_EXPECT_OK(al_delete_data(dop, leaf)); + AL_ASSERT_OK(al_end_action(dop)); + + int rop = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", READ_OP, &rop)); + std::vector shape; + std::vector a, b, c; + AL_EXPECT_OK(al_contract::read_data(rop, "leaf_a", 0, &shape, &a)); + AL_EXPECT_OK(al_contract::read_data(rop, "leaf_b", 0, &shape, &b)); + AL_EXPECT_OK(al_contract::read_data(rop, "leaf_c", 0, &shape, &c)); + AL_ASSERT_OK(al_end_action(rop)); + EXPECT_EQ(a.at(0), al_contract::kEmptyDouble); + EXPECT_EQ(b.at(0), al_contract::kEmptyDouble) + << "the second delete of a session must take effect too"; + EXPECT_EQ(c.at(0), 1.0) << "a leaf nobody deleted must survive"; + al_close_pulse(pctx, CLOSE_PULSE); +} // =========================================================================== // al_get_occurrences. From 6cfff40e6f7f3177ab007f9edf2d1012622cbaad Mon Sep 17 00:00:00 2001 From: YohannMrg <98689112+yohannmarguier@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:24:42 +0200 Subject: [PATCH 3/4] Match delete paths per segment, so a path may cross an AOS (issue #63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut matched dataset names by string prefix plus a continuation character. That is right for every path the HLI issues, but it matches nothing at all for a path that traverses an array of structures: `outer/inner` mangles to `outer&inner` while the dataset is named `outer[]&inner&leaf`. The call then deleted nothing and reported success — in miniature, the same "argument accepted, ignored, success reported" state this issue exists to remove. Confirmed by test before changing the rule. Match per segment instead: each segment of the requested path matches a dataset-name segment with or without its `[]` AOS suffix, and the name's remaining segments are its subtree. Segment-wise still tells a real child from a same-prefix stranger (`time` vs `time_slice[]&x`, `code` vs `code_name`), and it gives an AOS-crossing path the one meaning the ABI leaves available: `deleteData` receives an OperationContext, never an arraystruct one, so no element can be named and the path can only mean "that subtree in every element" — exactly what the tensorized layout holds. (cherry picked from commit 1c00c855bad55045c32ed22ae7e2610a1bf24be1) --- .../adr/0001-al-delete-data-path-semantics.md | 18 ++++-- src/hdf5/hdf5_writer.cpp | 63 ++++++++++++++----- tests/contract/test_structured_data.cpp | 44 +++++++++++++ 3 files changed, 104 insertions(+), 21 deletions(-) diff --git a/docs/adr/0001-al-delete-data-path-semantics.md b/docs/adr/0001-al-delete-data-path-semantics.md index 49de0621..e31ef783 100644 --- a/docs/adr/0001-al-delete-data-path-semantics.md +++ b/docs/adr/0001-al-delete-data-path-semantics.md @@ -20,10 +20,20 @@ an absent path is a successful no-op. `HDF5Writer::deleteSubtree`. An IDS group is a flat namespace of "tensorized" dataset names — the DD path with `/` → `&`, array-of-structures nodes suffixed `[]`, one `_SHAPE` companion per non-scalar leaf, one `…[]&AOS_SHAPE` per -dynamic AOS — so the subtree rooted at a DD path is exactly the set of names -that are the mangled path, its `_SHAPE` companion, or that continue with `&` (a -structure child) or `[]` (an AOS element index). The continuation character is -what keeps `time` from taking `time_slice[]&x` with it. +dynamic AOS — so `time_slice/global_quantities/ip` is stored as +`time_slice[]&global_quantities&ip`, flattened over the AOS index. Matching is +therefore **per segment**, not by string prefix: each segment of the requested +path matches a name segment with or without the `[]` suffix, and the name's +remaining segments are its subtree. Segment-wise is what tells a real child +from a same-prefix stranger (deleting `time` must not take `time_slice[]&x`, +deleting `code` must not take `code_name`) while still letting a path address a +node *inside* an AOS. That last case has only one available meaning: the ABI +hands `deleteData` an `OperationContext`, never an arraystruct one, so there is +no way to name an element and `time_slice/global_quantities` can only mean "that +subtree in every element" — which is exactly what the tensorized layout stores. +A raw-prefix rule matched no dataset at all for such a path and returned +success, i.e. it reproduced in miniature the very failure mode this ADR +rejects. **2. Occurrence-wide delete is intended: reject a non-empty `path`.** Rejected. It is the cheaper change, but it is not what the ABI says and not what the other diff --git a/src/hdf5/hdf5_writer.cpp b/src/hdf5/hdf5_writer.cpp index 3782aafc..ba2e9a4c 100644 --- a/src/hdf5/hdf5_writer.cpp +++ b/src/hdf5/hdf5_writer.cpp @@ -61,27 +61,56 @@ void HDF5Writer::close_file_handler(std::string external_link_name, std::unorder // An IDS group is a flat namespace of "tensorized" dataset names: the DD path // with '/' replaced by '&', every array-of-structures node suffixed "[]", one // "_SHAPE" companion per non-scalar leaf and one "…[]&AOS_SHAPE" per dynamic -// AOS. So the subtree rooted at a DD path is exactly the set of dataset names -// that are the mangled path itself, its "_SHAPE" companion, or that continue -// with '&' (a structure child) or "[]" (an AOS element index). +// AOS. So "time_slice/global_quantities/ip" is stored as +// "time_slice[]&global_quantities&ip", flattened over the AOS index. // -// The continuation character is what separates a subtree from a same-prefix -// sibling: deleting "time" must not touch "time_slice[]&…". +// Matching therefore has to be per segment, not by raw string prefix: each +// segment of the requested path matches a dataset-name segment with or without +// the "[]" AOS suffix, and the dataset's remaining segments are its subtree. +// Segment-wise is what tells a real child from a same-prefix stranger — +// deleting "time" must not touch "time_slice[]&x", and deleting "code" must not +// touch "code_name" — while still letting a path address a node *inside* an +// AOS, which the C ABI gives no way to do per element: al_delete_data takes an +// OperationContext, so "time_slice/global_quantities" can only mean "that +// subtree in every element", exactly as the tensorized layout stores it. +static std::vector < std::string > split_on_ampersand(const std::string & path) +{ + std::vector < std::string > segments; + std::string::size_type start = 0; + while (true) { + const std::string::size_type sep = path.find('&', start); + if (sep == std::string::npos) { + segments.push_back(path.substr(start)); + return segments; + } + segments.push_back(path.substr(start, sep - start)); + start = sep + 1; + } +} + static bool is_in_subtree(const std::string & dataset_name, const std::string & mangled_path) { - if (dataset_name.size() < mangled_path.size() || - dataset_name.compare(0, mangled_path.size(), mangled_path) != 0) + const std::vector < std::string > name_segments = split_on_ampersand(dataset_name); + const std::vector < std::string > wanted_segments = split_on_ampersand(mangled_path); + if (name_segments.size() < wanted_segments.size()) + return false; + + for (size_t i = 0; i < wanted_segments.size(); i++) { + std::string segment = name_segments[i]; + if (segment.size() >= 2 && segment.compare(segment.size() - 2, 2, "[]") == 0) + segment.erase(segment.size() - 2); //an AOS node: "time_slice[]" -> "time_slice" + if (segment == wanted_segments[i]) + continue; + //The addressed node's own "_SHAPE" companion is the one case where the + //last segment may carry a suffix. A deeper AOS's "AOS_SHAPE" and a + //child's "…_SHAPE" are already covered: they are extra segments, or + //segments past the requested path. + if (i + 1 == wanted_segments.size() && name_segments.size() == wanted_segments.size() && + segment == wanted_segments[i] + "_SHAPE") + continue; return false; - const std::string continuation = dataset_name.substr(mangled_path.size()); - if (continuation.empty()) // the leaf's own dataset - return true; - if (continuation == "_SHAPE") // its shape companion - return true; - if (continuation[0] == '&') // a child of a structure - return true; - if (continuation.compare(0, 2, "[]") == 0) // "[]&…", "[]&AOS_SHAPE" - return true; - return false; + } + return true; } void HDF5Writer::invalidateGroupMembers() diff --git a/tests/contract/test_structured_data.cpp b/tests/contract/test_structured_data.cpp index 4cbd2bab..3683b3db 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -704,6 +704,9 @@ TEST_F(DeleteKnownDefects, AsciiDeleteIsCurrentlyANoOp) { // names ('/' -> '&', AOS nodes suffixed "[]"), so "time" and // "time_slice[]&x" are neighbours in one namespace and a prefix match // alone would take the second with the first. +// 4. Paths that traverse an AOS. The same flattening means "outer/inner" is +// stored as "outer[]&inner&…", so matching has to be per segment or such +// a path silently matches nothing. // One non-empty-path delete of a single leaf, observed on three axes: the // sibling leaf, the occurrence's own pulse file, and the external link the // master file holds for that occurrence. @@ -869,6 +872,47 @@ TEST_F(Hdf5Delete, SamePrefixSiblingsSurviveASubtreeDelete) { al_close_pulse(pctx, CLOSE_PULSE); } +// A path may address a node *inside* an AOS. The C ABI gives no way to say +// which element — al_delete_data takes an OperationContext, not an +// arraystruct one — so "outer/inner" can only mean "that subtree in every +// element", which is exactly how the tensorized layout stores it +// ("outer[]&inner&leaf", flattened over the AOS index). Raw prefix matching +// missed these entirely and reported success, so the call was a silent no-op. +TEST_F(Hdf5Delete, PathThroughAnAosDeletesTheSubtreeInEveryElement) { + const int pctx = open(); + int op = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op)); + int size = 2; + int aos = -1; + AL_ASSERT_OK(al_begin_arraystruct_action(op, "outer", "", &size, &aos)); + AL_EXPECT_OK(al_contract::write_data(aos, "inner/leaf", {}, {1.0})); + AL_EXPECT_OK(al_contract::write_data(aos, "keep", {}, {3.0})); + AL_EXPECT_OK(al_iterate_over_arraystruct(aos, 1)); + AL_EXPECT_OK(al_contract::write_data(aos, "inner/leaf", {}, {2.0})); + AL_EXPECT_OK(al_contract::write_data(aos, "keep", {}, {4.0})); + AL_ASSERT_OK(al_end_action(aos)); + AL_EXPECT_OK(al_delete_data(op, "outer/inner")); + AL_ASSERT_OK(al_end_action(op)); + + int rop = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", READ_OP, &rop)); + int rsize = -1; + int raos = -1; + AL_ASSERT_OK(al_begin_arraystruct_action(rop, "outer", "", &rsize, &raos)); + ASSERT_EQ(rsize, 2); + std::vector shape; + std::vector leaf, keep; + AL_EXPECT_OK(al_contract::read_data(raos, "inner/leaf", 0, &shape, &leaf)); + AL_EXPECT_OK(al_contract::read_data(raos, "keep", 0, &shape, &keep)); + AL_ASSERT_OK(al_end_action(raos)); + AL_ASSERT_OK(al_end_action(rop)); + EXPECT_EQ(leaf.at(0), al_contract::kEmptyDouble) + << "a path through an AOS must delete its subtree in every element"; + EXPECT_EQ(keep.at(0), 3.0) << "a sibling inside the same AOS element must survive"; + + al_close_pulse(pctx, CLOSE_PULSE); +} + TEST_F(Hdf5Delete, DeletingAnAbsentPathChangesNothing) { const int pctx = open(); int op = -1; From 38e26f4e1d5521282b32a004abf593f9447974e2 Mon Sep 17 00:00:00 2001 From: YohannMrg <98689112+yohannmarguier@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:28:04 +0200 Subject: [PATCH 4/4] Address the code review on the al_delete_data change (issue #63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Standards axis: - The group listing now validates itself against the group's live link count instead of relying on four hand-placed invalidateGroupMembers() calls on the write paths. Only a write can add a link, and a write can only add, so an unchanged count means an unchanged set of names — which removes the latent coupling where any future dataset-creating path had to remember to invalidate. - unlinkFromGroup() owns both the H5Ldelete loop and keeping the cache in step, so deleteSubtree no longer reassigns the writer's own vector through a returned reference. - The delete error builds its message as a std::string rather than through a fixed-size buffer: a dataset name is a caller-supplied DD path, which the src/hdf5 `char error_message[200]` idiom cannot bound. - Both H5 failures in the listing name their own function. - Hdf5Delete's tests all go through the fixture now; the free observation helper that duplicated three of its members (and skipped make_legacy_tree, unlike every other on-disk path in the file) is gone. Spec axis: - al_lowlevel.h's per-backend summary said memory does "node and subtree" where only the AOS case is characterized, and omitted UDA entirely. - The ADR records the two behaviour changes the issue did not itself measure — master-link removal on an empty-path delete, and deleting an IDS file that is closed but still on disk — rather than leaving them silent. The one substantive fidelity finding, that a path below an AOS silently matched nothing, was already fixed in the preceding commit; the review confirms it independently. (cherry picked from commit 0250431484a3f1c71fd1a58f6e6807f5b130a6c3) --- .../adr/0001-al-delete-data-path-semantics.md | 12 ++- include/al_lowlevel.h | 14 ++-- src/hdf5/hdf5_writer.cpp | 58 +++++++------ src/hdf5/hdf5_writer.h | 16 ++-- tests/contract/test_structured_data.cpp | 83 ++++++------------- 5 files changed, 89 insertions(+), 94 deletions(-) diff --git a/docs/adr/0001-al-delete-data-path-semantics.md b/docs/adr/0001-al-delete-data-path-semantics.md index e31ef783..97ad6f83 100644 --- a/docs/adr/0001-al-delete-data-path-semantics.md +++ b/docs/adr/0001-al-delete-data-path-semantics.md @@ -66,7 +66,7 @@ Checked by reading each `deleteData` and pinned in `tests/contract/` | Backend | node | subtree | whole DATAOBJECT (`path == ""`) | |---|---|---|---| | HDF5 | ✅ | ✅ | ✅ — deletes `.h5` *and* the master-file link | -| Memory | ✅ | ✅ (AOS node) | ❌ no code path for it — xfail | +| Memory | ✅ | ✅ (AOS node; a plain non-AOS structure is uncharacterized) | ❌ no code path for it — xfail | | MDSplus | ✅ | ❌ `%TREE-W-NNF` — xfail | ❌ `%TREE-W-NNF` — xfail | | ASCII | ❌ empty body | ❌ | ❌ | | Flexbuffers | ❌ empty body | ❌ | ❌ | @@ -95,7 +95,15 @@ reach it. A plain (non-AOS) structure path on Memory is not characterized. - **The master-file link now follows the file.** A whole-DATAOBJECT delete removes the external link as well, so the master file no longer advertises an occurrence whose backing file is gone — the second half of what issue #63 - measured. + measured. Issue #63 raises the dangling link under option 2 only, but it is + listed there as part of what was measured on today's code, so it is fixed + either way. +- **A whole-DATAOBJECT delete now also removes an IDS file that was closed but + still on disk.** The old code reopened such a file and then marked it closed + without deleting it — leaking the handle and leaving the data in place. The + issue never measured this (through a `WRITE_OP` the file is always open, so + the branch was unreachable in the reproduction), but the empty-path contract + cannot hold with it in place. - A whole-IDS delete lists the IDS group's link names once per delete rather than once per leaf (`HDF5Writer::groupMembers`, invalidated by anything that can add a dataset). Without that cache the HLI's leaf-by-leaf traversal would diff --git a/include/al_lowlevel.h b/include/al_lowlevel.h index 3e93148a..2450c334 100644 --- a/include/al_lowlevel.h +++ b/include/al_lowlevel.h @@ -426,11 +426,15 @@ extern "C" Backend support for the three granularities is uneven, and a backend that cannot honour `path` does not report that it could not: ASCII and - flexbuffers ignore the call entirely, memory implements node and subtree - deletes but not the DATAOBJECT one, MDSplus implements node deletes only. - HDF5 implements all three. The per-backend matrix, and why the argument is - specified this way rather than as an occurrence-wide delete, are recorded - in docs/adr/0001-al-delete-data-path-semantics.md. + flexbuffers ignore the call entirely, memory implements node and + array-of-structures deletes but not the DATAOBJECT one, MDSplus implements + node deletes only. HDF5 implements all three. UDA forwards `path` to + whichever backend it is fronting: in fetch mode to the local one, and in + remote mode to a server plugin that need not implement delete at all (the + reference plugin does not, and refuses cleanly). The per-backend matrix, + and why the argument is specified this way rather than as an + occurrence-wide delete, are recorded in + docs/adr/0001-al-delete-data-path-semantics.md. @param[in] ctx operation context id (from al_plugin_begin_global_action() or al_plugin_begin_slice_action()) @param[in] path path of the data structure element to delete (suppress the whole subtree); empty means the whole DATAOBJECT diff --git a/src/hdf5/hdf5_writer.cpp b/src/hdf5/hdf5_writer.cpp index ba2e9a4c..e4dd33e5 100644 --- a/src/hdf5/hdf5_writer.cpp +++ b/src/hdf5/hdf5_writer.cpp @@ -119,23 +119,50 @@ void HDF5Writer::invalidateGroupMembers() group_members.clear(); } -std::vector < std::string > &HDF5Writer::groupMembers(hid_t gid) +// The cached listing is trusted only while it still accounts for every link in +// the group. Nothing but a write can add one, and a write can only add, so an +// unchanged link count means an unchanged set of names — which spares every +// write path from having to remember to invalidate this. +const std::vector < std::string > &HDF5Writer::groupMembers(hid_t gid) { - if (group_members_gid != gid) { + H5G_info_t group_info; + if (H5Gget_info(gid, &group_info) < 0) + throw ALBackendException("HDF5Backend: H5Gget_info has failed in HDF5Writer::groupMembers()", LOG); + + if (group_members_gid != gid || group_members.size() != (size_t) group_info.nlinks) { group_members.clear(); H5L_iterate_t collect = [](hid_t, const char *name, const H5L_info_t *, void *op_data) -> herr_t { static_cast < std::vector < std::string > *>(op_data)->push_back(name); return 0; }; if (H5Literate(gid, H5_INDEX_NAME, H5_ITER_NATIVE, NULL, collect, &group_members) < 0) { - group_members.clear(); - throw ALBackendException("HDF5Backend: H5Literate has failed in HDF5Writer::deleteData()", LOG); + invalidateGroupMembers(); + throw ALBackendException("HDF5Backend: H5Literate has failed in HDF5Writer::groupMembers()", LOG); } group_members_gid = gid; } return group_members; } +void HDF5Writer::unlinkFromGroup(hid_t gid, const std::vector < std::string > &names) +{ + for (const std::string & name : names) { + // H5Ldelete only unlinks: the freed space stays in the file until it is + // repacked, so a delete never shrinks .h5. + if (H5Ldelete(gid, name.c_str(), H5P_DEFAULT) < 0) { + invalidateGroupMembers(); + //A dataset name is a caller-supplied DD path, so it is built into + //the message as a string rather than through a fixed-size buffer. + throw ALBackendException("HDF5Backend: unable to delete HDF5 dataset: " + name, LOG); + } + } + //Keep the cached listing in step, so a run of deletes still lists the group + //only once. This is the only code that removes a link from the group. + for (const std::string & name : names) + group_members.erase(std::remove(group_members.begin(), group_members.end(), name), + group_members.end()); +} + void HDF5Writer::deleteData(OperationContext * ctx, const std::string & path, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path) { if (file_id == -1) @@ -170,27 +197,12 @@ void HDF5Writer::deleteSubtree(hid_t gid, const std::string & path) write_buffers(); close_datasets(); //also clears the existing_data_sets H5Lexists cache - std::vector < std::string > &members = groupMembers(gid); std::vector < std::string > doomed; - std::vector < std::string > kept; - for (const std::string & name : members) { + for (const std::string & name : groupMembers(gid)) { if (is_in_subtree(name, mangled_path)) doomed.push_back(name); - else - kept.push_back(name); - } - - for (const std::string & name : doomed) { - // H5Ldelete only unlinks: the freed space stays in the file until it is - // repacked, so a delete never shrinks .h5. - if (H5Ldelete(gid, name.c_str(), H5P_DEFAULT) < 0) { - invalidateGroupMembers(); - char error_message[300]; - snprintf(error_message, sizeof(error_message), "Unable to delete HDF5 dataset: %s\n", name.c_str()); - throw ALBackendException(error_message, LOG); - } } - members = std::move(kept); + unlinkFromGroup(gid, doomed); } void HDF5Writer::deleteOccurrence(OperationContext * ctx, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path) @@ -289,7 +301,7 @@ void HDF5Writer::create_IDS_group(OperationContext * ctx, hid_t file_id, std::un throw ALBackendException(error_message, LOG); } } - close_group(ctx); //also invalidates the cached group listing + close_group(ctx); hid_t loc_id = hdf5_utils.createOrOpenHDF5Group(ctx->getDataobjectName().c_str(), IDS_file_id); if (! (loc_id >= 0)) throw ALBackendException("HDF5Backend: unexpected value for loc_id in HDF5Writer::create_IDS_group()", LOG); @@ -398,7 +410,6 @@ int HDF5Writer::readTimedAOSShape(hid_t loc_id, std::string &tensorized_path, co void HDF5Writer::beginWriteArraystructAction(ArraystructContext * ctx, int *size) { - invalidateGroupMembers(); //may add the AOS_SHAPE dataset to the group HDF5Utils hdf5_utils; OperationContext *opctx = ctx->getOperationContext(); hid_t gid = -1; @@ -462,7 +473,6 @@ ArraystructContext* HDF5Writer::getDynamicAOS(Context * ctx) { void HDF5Writer::write_ND_Data(Context * ctx, std::string & att_name, std::string & timebasename, int datatype, int dim, int *size, void *data) { - invalidateGroupMembers(); //may add the value and _SHAPE datasets to the group std::string & dataset_name = att_name; std::replace(dataset_name.begin(), dataset_name.end(), '/', '&'); // character '/' is not supported in datasets names std::replace(timebasename.begin(), timebasename.end(), '/', '&'); diff --git a/src/hdf5/hdf5_writer.h b/src/hdf5/hdf5_writer.h index 71014d35..7acb17eb 100644 --- a/src/hdf5/hdf5_writer.h +++ b/src/hdf5/hdf5_writer.h @@ -22,12 +22,13 @@ class HDF5Writer { int homogeneous_time; std::unordered_map < OperationContext *, hid_t> IDS_group_id; - // Link names of one IDS group, cached for the duration of a run of - // deleteData() calls. Reset by invalidateGroupMembers() on anything that - // can add a dataset to a group or swap the group itself, so an over-eager - // reset is always the safe direction. Without it a whole-IDS delete (the - // HLI issues one al_delete_data per DD leaf) would re-list the group once - // per leaf. + // Link names of one IDS group, cached so that a run of deleteData() calls + // lists the group once instead of once per call — the HLI issues one + // al_delete_data per DD leaf, so re-listing per call would be quadratic in + // the size of the IDS. The cache validates itself against the group's live + // link count rather than relying on write paths to invalidate it: only a + // write can add a link, and the only code that removes one + // (unlinkFromGroup) keeps the cache in step itself. hid_t group_members_gid; std::vector < std::string > group_members; @@ -41,7 +42,8 @@ class HDF5Writer { int getDynamic_AOS_slices_extension(Context *ctx); void deleteSubtree(hid_t gid, const std::string & path); void deleteOccurrence(OperationContext * ctx, hid_t file_id, std::unordered_map < std::string, hid_t > &opened_IDS_files, std::string & files_directory, std::string & relative_file_path); - std::vector < std::string > &groupMembers(hid_t gid); + const std::vector < std::string > &groupMembers(hid_t gid); + void unlinkFromGroup(hid_t gid, const std::vector < std::string > &names); void invalidateGroupMembers(); int getDynamic_slices_extension(Context *ctx, int timed_AOS_index, int time_vector_length); ArraystructContext* getDynamicAOS(Context * ctx); diff --git a/tests/contract/test_structured_data.cpp b/tests/contract/test_structured_data.cpp index 3683b3db..585ac5fe 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -707,15 +707,6 @@ TEST_F(DeleteKnownDefects, AsciiDeleteIsCurrentlyANoOp) { // 4. Paths that traverse an AOS. The same flattening means "outer/inner" is // stored as "outer[]&inner&…", so matching has to be per segment or such // a path silently matches nothing. -// One non-empty-path delete of a single leaf, observed on three axes: the -// sibling leaf, the occurrence's own pulse file, and the external link the -// master file holds for that occurrence. -struct Hdf5LeafDeleteObservation { - double sibling_value; // leaf_b read back after al_delete_data("leaf_a") - bool ids_file_exists; // .h5 still on disk - bool occurrence_listed; // master file still links the occurrence -}; - // Located by name rather than by rebuilding the backend's own on-disk path // strategy (hdf5_utils.cpp pulseFilePathFactory), which the test has no // business restating. @@ -730,49 +721,6 @@ inline bool file_exists_under(const std::string& root, const std::string& name) return false; } -Hdf5LeafDeleteObservation hdf5_observe_leaf_delete(al_contract::TempBase& base, - const PulseId& pulse) { - const std::string uri = al_contract::build_uri(HDF5_BACKEND, base.str(), pulse); - int pctx = -1; - EXPECT_EQ(al_begin_dataentry_action(uri.c_str(), FORCE_CREATE_PULSE, &pctx).code, 0); - int op = -1; - EXPECT_EQ(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op).code, 0); - EXPECT_EQ(al_contract::write_data(op, "leaf_a", {}, {11.0}).code, 0); - EXPECT_EQ(al_contract::write_data(op, "leaf_b", {}, {22.0}).code, 0); - EXPECT_EQ(al_delete_data(op, "leaf_a").code, 0); - EXPECT_EQ(al_end_action(op).code, 0); - - // Reopening a READ on the occurrence may itself fail — if the delete - // removed the whole occurrence's group/link/file there is nothing left to - // open. That failure is just as much proof the sibling is gone as reading - // back the sentinel would be. - int rop = -1; - al_status_t open_read = al_begin_global_action(pctx, kIds, "", READ_OP, &rop); - Hdf5LeafDeleteObservation obs{al_contract::kEmptyDouble, false, false}; - if (open_read.code == 0) { - std::vector shape; - std::vector bb; - EXPECT_EQ(al_contract::read_data(rop, "leaf_b", 0, &shape, &bb).code, 0); - EXPECT_EQ(al_end_action(rop).code, 0); - if (!bb.empty()) obs.sibling_value = bb[0]; - } - - // al_get_occurrences enumerates the master file's links live - // (hdf5_reader.cpp H5Literate over master_file_id), so it reports the link - // whether or not the file it points at still exists — exactly the dangling - // state issue #63 measured. - int* occ = nullptr; - int n = -1; - if (al_get_occurrences(pctx, kIds, &occ, &n).code == 0 && n > 0) - obs.occurrence_listed = true; - free(occ); - - obs.ids_file_exists = file_exists_under(base.str(), std::string(kIds) + ".h5"); - - al_close_pulse(pctx, CLOSE_PULSE); - return obs; -} - class Hdf5Delete : public ::testing::Test { protected: al_contract::TempBase base_; @@ -791,6 +739,11 @@ class Hdf5Delete : public ::testing::Test { } // True iff master.h5 still holds an external link for the occurrence. + // al_get_occurrences enumerates those links live (hdf5_reader.cpp + // H5Literate over master_file_id) and never stats what they point at, so + // it reports a link whose file is gone — which is what makes it the oracle + // for the dangling state issue #63 measured, independently of + // ids_file_exists(). bool occurrence_listed(int pctx) const { int* occ = nullptr; int n = -1; @@ -801,14 +754,32 @@ class Hdf5Delete : public ::testing::Test { }; TEST_F(Hdf5Delete, LeafDeleteKeepsOccurrenceFileAndMasterLink) { - const Hdf5LeafDeleteObservation obs = hdf5_observe_leaf_delete(base_, pulse_); - EXPECT_EQ(obs.sibling_value, 22.0) + const int pctx = open(); + int op = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", WRITE_OP, &op)); + AL_EXPECT_OK(al_contract::write_data(op, "leaf_a", {}, {11.0})); + AL_EXPECT_OK(al_contract::write_data(op, "leaf_b", {}, {22.0})); + AL_EXPECT_OK(al_delete_data(op, "leaf_a")); + AL_ASSERT_OK(al_end_action(op)); + + int rop = -1; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", READ_OP, &rop)); + std::vector shape; + std::vector a, b; + AL_EXPECT_OK(al_contract::read_data(rop, "leaf_a", 0, &shape, &a)); + AL_EXPECT_OK(al_contract::read_data(rop, "leaf_b", 0, &shape, &b)); + AL_ASSERT_OK(al_end_action(rop)); + + EXPECT_EQ(a.at(0), al_contract::kEmptyDouble) << "the named leaf must be gone"; + EXPECT_EQ(b.at(0), 22.0) << "a leaf-granularity delete must not remove sibling fields"; - EXPECT_TRUE(obs.ids_file_exists) + EXPECT_TRUE(ids_file_exists()) << "a leaf-granularity delete must not remove the occurrence's pulse file"; - EXPECT_TRUE(obs.occurrence_listed) + EXPECT_TRUE(occurrence_listed(pctx)) << "the occurrence, and so the master file's link to it, must survive a " "leaf-granularity delete"; + + al_close_pulse(pctx, CLOSE_PULSE); } TEST_F(Hdf5Delete, RootDeleteRemovesOccurrenceFileAndMasterLink) {