Fix HDF5 al_delete_data ignoring its path argument

docs/adr/0001-al-delete-data-path-semantics.md

@@ -0,0 +1,110 @@
+---
+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 `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
+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 `<ids>.h5` *and* the master-file link |
+| 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 | ❌ | ❌ |
+| 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 `<ids>.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 `<ids>.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. 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
+  be quadratic in the size of the IDS.

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,

include/al_lowlevel.h

@@ -415,8 +415,29 @@ 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
+     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)
+     @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,

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)

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,112 @@ 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 "time_slice/global_quantities/ip" is stored as
+// "time_slice[]&global_quantities&ip", flattened over the AOS index.
+//
+// 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)
+{
+    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;
+    }
+    return true;
+}
+
+void HDF5Writer::invalidateGroupMembers()
+{
+    group_members_gid = -1;
+    group_members.clear();
+}
+
+// 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)
+{
+    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) {
+            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 <ids>.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)
         throw ALBackendException("HDF5Backend: master file not opened in HDF5Writer::deleteData()", LOG); //the master file is assumed to be opened
@@ -69,33 +174,58 @@ 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 > doomed;
+    for (const std::string & name : groupMembers(gid)) {
+        if (is_in_subtree(name, mangled_path))
+            doomed.push_back(name);
+    }
+    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)
+{
     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);
     }
 }
 
@@ -194,6 +324,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()) {