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..97ad6f83 --- /dev/null +++ b/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 `.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 `.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. 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. 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..2450c334 100644 --- a/include/al_lowlevel.h +++ b/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, 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..e4dd33e5 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,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 .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()) { diff --git a/src/hdf5/hdf5_writer.h b/src/hdf5/hdf5_writer.h index 70caf417..7acb17eb 100644 --- a/src/hdf5/hdf5_writer.h +++ b/src/hdf5/hdf5_writer.h @@ -21,6 +21,16 @@ class HDF5Writer { int homogeneous_time; std::unordered_map < OperationContext *, hid_t> IDS_group_id; + + // 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; int slice_mode; @@ -30,6 +40,11 @@ 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); + 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); @@ -44,7 +59,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 9a59e6e7..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` + tripwire `DeleteKnownDefects.Hdf5LeafDeleteCurrentlyWipesWholeOccurrence`; 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,7 +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` | | 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..585ac5fe 100644 --- a/tests/contract/test_structured_data.cpp +++ b/tests/contract/test_structured_data.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include @@ -429,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. @@ -461,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, @@ -607,53 +607,8 @@ 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 ---------- -double hdf5_leaf_delete_sibling_survives(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 — 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. - int rop = -1; - al_status_t open_read = al_begin_global_action(pctx, kIds, "", READ_OP, &rop); - double result = al_contract::kEmptyDouble; - 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]; - } - al_close_pulse(pctx, CLOSE_PULSE); - return result; -} - -TEST_F(DeleteKnownDefects, DISABLED_Hdf5LeafDeleteLeavesSiblingIntact) { - EXPECT_EQ(hdf5_leaf_delete_sibling_survives(base_, pulse_), 22.0) - << "a leaf-granularity delete must not remove sibling fields"; -} - -TEST_F(DeleteKnownDefects, Hdf5LeafDeleteCurrentlyWipesWholeOccurrence) { - EXPECT_EQ(hdf5_leaf_delete_sibling_survives(base_, pulse_), - 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)"; -} - // --- Memory: no code path implements DATAOBJECT-root delete ---------------- bool memory_root_delete_clears_everything(al_contract::TempBase& base, const PulseId& pulse) { @@ -731,6 +686,258 @@ TEST_F(DeleteKnownDefects, AsciiDeleteIsCurrentlyANoOp) { } +// =========================================================================== +// 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. +// 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. +// 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; +} + +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. + // 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; + const bool ok = al_get_occurrences(pctx, kIds, &occ, &n).code == 0 && n > 0; + free(occ); + return ok; + } +}; + +TEST_F(Hdf5Delete, LeafDeleteKeepsOccurrenceFileAndMasterLink) { + 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(ids_file_exists()) + << "a leaf-granularity delete must not remove the occurrence's pulse file"; + 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) { + 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); +} + +// 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; + 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; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", READ_OP, &rop)); + std::vector shape; + 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)); + + 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"; + + 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; + 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; + AL_ASSERT_OK(al_begin_global_action(pctx, kIds, "", READ_OP, &rop)); + std::vector shape; + std::vector a; + 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); +} + +// 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. // ===========================================================================