From bc76ea00411310af26fb4250ca63191e11d9880f Mon Sep 17 00:00:00 2001 From: "Christopher M. Pierce" Date: Thu, 17 Sep 2026 00:48:48 -0700 Subject: [PATCH 1/3] add test to reproduce double close --- tests/test_read.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/test_read.c b/tests/test_read.c index 096c31d..b655a6b 100644 --- a/tests/test_read.c +++ b/tests/test_read.c @@ -427,6 +427,52 @@ void test_file_based_multiple_percent_t(void) { pmd_close_series(series); } +static herr_t count_hdf5_errors(hid_t estack, void *client_data) { + (void)estack; + (*(int *)client_data)++; + return 0; +} + +/* Test: Opening and closing series releases each HDF5 file handle exactly once + * Files: tests/data/file_based_series/data_{0,%T}.h5, file_based_iteration_format_mismatch.h5, + * valid_multiple_iterations.h5 + * Tests: No HDF5 errors (e.g. closing an already closed file) and no leaked file handles */ +void test_open_series_no_double_close(void) { + pmd_series *series; + int hdf5_error_count = 0; + ssize_t open_files_before = H5Fget_obj_count((hid_t)H5F_OBJ_ALL, H5F_OBJ_FILE); + + /* Count HDF5 errors instead of silencing them; assertions run after the handler is restored */ + H5Eset_auto2(H5E_DEFAULT, count_hdf5_errors, &hdf5_error_count); + + /* FILE_BASED, specific file */ + pmd_status file_based_result = pmd_open_series("tests/data/file_based_series/data_0.h5", &series, PMD_RDONLY); + pmd_close_series(series); + + /* FILE_BASED, %T pattern */ + pmd_status pattern_result = pmd_open_series("tests/data/file_based_series/data_%T.h5", &series, PMD_RDONLY); + pmd_close_series(series); + + /* FILE_BASED, error after metadata has been read */ + pmd_status mismatch_result = pmd_open_series("tests/data/file_based_iteration_format_mismatch.h5", + &series, PMD_RDONLY); + pmd_close_series(series); + + /* GROUP_BASED, series keeps the file open */ + pmd_status group_based_result = pmd_open_series("tests/data/valid_multiple_iterations.h5", &series, PMD_RDONLY); + pmd_close_series(series); + + ssize_t open_files_after = H5Fget_obj_count((hid_t)H5F_OBJ_ALL, H5F_OBJ_FILE); + H5Eset_auto2(H5E_DEFAULT, NULL, NULL); + + TEST_ASSERT_EQUAL_INT(PMD_SUCCESS, file_based_result); + TEST_ASSERT_EQUAL_INT(PMD_SUCCESS, pattern_result); + TEST_ASSERT_EQUAL_INT(PMD_ERROR_FILE_FORMAT, mismatch_result); + TEST_ASSERT_EQUAL_INT(PMD_SUCCESS, group_based_result); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, hdf5_error_count, "HDF5 reported errors while opening/closing series"); + TEST_ASSERT_EQUAL_INT64((int64_t)open_files_before, (int64_t)open_files_after); +} + /* Test: Group-based series with multiple iterations * File: tests/data/valid_multiple_iterations.h5 * Tests: Basic group-based iteration enumeration */ @@ -2720,6 +2766,7 @@ int main(void) { RUN_TEST(test_file_based_series_pattern_path); RUN_TEST(test_file_based_series_with_other_files); RUN_TEST(test_file_based_multiple_percent_t); + RUN_TEST(test_open_series_no_double_close); RUN_TEST(test_group_based_series_multiple_iterations); RUN_TEST(test_group_based_non_matching_groups); RUN_TEST(test_iteration_format_prefix_suffix); From c752d6864ff3a441ccbef6f1b1b2a93dcfed426b Mon Sep 17 00:00:00 2001 From: "Christopher M. Pierce" Date: Thu, 17 Sep 2026 00:54:53 -0700 Subject: [PATCH 2/3] remove file handle management from read_series_metadata_from_file --- parcel.h | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/parcel.h b/parcel.h index b4140bf..2df37b1 100644 --- a/parcel.h +++ b/parcel.h @@ -1658,7 +1658,7 @@ static pmd_status read_series_metadata_from_file(hid_t file_id, pmd_series *seri if (status != PMD_SUCCESS) { return status; } - /* Parse iteration encoding and handle file lifecycle */ + /* Parse iteration encoding (file_id is owned by the caller and is not closed here) */ if (strcmp(iter_encoding_str, "fileBased") == 0) { series->iteration_encoding = PMD_FILE_BASED; @@ -1677,10 +1677,6 @@ static pmd_status read_series_metadata_from_file(hid_t file_id, pmd_series *seri return PMD_ERROR_FILE_FORMAT; } free_iteration_pattern(&pattern_check); - - /* Don't keep file open for fileBased */ - H5Fclose(file_id); - series->file_id = -1; } else if (strcmp(iter_encoding_str, "groupBased") == 0) { series->iteration_encoding = PMD_GROUP_BASED; @@ -1689,9 +1685,6 @@ static pmd_status read_series_metadata_from_file(hid_t file_id, pmd_series *seri free(iter_encoding_str); return PMD_ERROR_FILE_FORMAT; } - - /* For groupBased, keep file open */ - series->file_id = file_id; } else { free(iter_encoding_str); return PMD_ERROR_FILE_FORMAT; @@ -2008,8 +2001,9 @@ pmd_status pmd_open_series(const char *filename, pmd_series **series_out, pmd_ac free_iteration_pattern(&pattern_info); } - /* Keep file open for group-based */ + /* Keep file open for group-based; series now owns the handle */ series->file_id = file_id; + file_id = -1; } /* If we are opening an existing file (must exist) */ else{ @@ -2036,13 +2030,20 @@ pmd_status pmd_open_series(const char *filename, pmd_series **series_out, pmd_ac status = PMD_ERROR_HDF5; goto cleanup; } - series->file_id = file_id; /* Read metadata from the opened file */ status = read_series_metadata_from_file(file_id, series, filename); if (status != PMD_SUCCESS) { goto cleanup; } + + /* Group-based series keep the file open; file-based series open iteration files on demand */ + if (series->iteration_encoding == PMD_GROUP_BASED) { + series->file_id = file_id; + } else { + H5Fclose(file_id); + } + file_id = -1; } /* For FILE_BASED series, set directory to parent of filename */ @@ -2088,7 +2089,7 @@ pmd_status pmd_open_series(const char *filename, pmd_series **series_out, pmd_ac } cleanup: - if (file_id >= 0 && series->file_id < 0) { + if (file_id >= 0) { H5Fclose(file_id); } if (status != PMD_SUCCESS) { From a7ad15f1919afd38e1da974cefd595148b68d5ba Mon Sep 17 00:00:00 2001 From: "Christopher M. Pierce" Date: Thu, 17 Sep 2026 00:57:16 -0700 Subject: [PATCH 3/3] error on pattern-based-filename matching group-based file --- parcel.h | 8 ++++++++ tests/test_write.c | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/parcel.h b/parcel.h index 2df37b1..dafcf6d 100644 --- a/parcel.h +++ b/parcel.h @@ -1942,6 +1942,14 @@ pmd_status pmd_open_series(const char *filename, pmd_series **series_out, pmd_ac goto cleanup; } + /* A %T filename pattern is only valid for file-based series */ + if (series->iteration_encoding != PMD_FILE_BASED) { + pmd_log(PMD_LOG_ERROR, "File '%s' matched pattern '%s' but is not a fileBased series", + iter_filename, filename); + status = PMD_ERROR_FILE_FORMAT; + goto cleanup; + } + /* Close file since we will not store for file-based mode */ H5Fclose(file_id); file_id = -1; diff --git a/tests/test_write.c b/tests/test_write.c index e760aa0..09d74f2 100644 --- a/tests/test_write.c +++ b/tests/test_write.c @@ -1009,6 +1009,28 @@ void test_invalid_pattern_ambiguous(void) { TEST_ASSERT_NOT_EQUAL(PMD_SUCCESS, result); } +/** + * Test: %T filename pattern that matches a group-based file fails + */ +void test_pattern_matching_group_based_file_fails(void) { + pmd_series *series; + pmd_status result; + + /* Create a group-based file whose name matches grp_%T.h5 */ + result = pmd_open_series(TEST_TEMP_DIR "/grp_0.h5", &series, PMD_TRUNC); + TEST_ASSERT_EQUAL_INT(PMD_SUCCESS, result); + pmd_close_series(series); + + ssize_t open_files_before = H5Fget_obj_count((hid_t)H5F_OBJ_ALL, H5F_OBJ_FILE); + + result = pmd_open_series(TEST_TEMP_DIR "/grp_%T.h5", &series, PMD_RDONLY); + TEST_ASSERT_EQUAL_INT(PMD_ERROR_FILE_FORMAT, result); + TEST_ASSERT_NULL(series); + + ssize_t open_files_after = H5Fget_obj_count((hid_t)H5F_OBJ_ALL, H5F_OBJ_FILE); + TEST_ASSERT_EQUAL_INT64((int64_t)open_files_before, (int64_t)open_files_after); +} + /** * Test: Various valid file-based iteration patterns * Tests multiple pattern formats in a parameterized style @@ -2708,6 +2730,7 @@ int main(void) { RUN_TEST(test_write_nonconsecutive_iterations_file_based); RUN_TEST(test_write_fails_no_parent_directory); RUN_TEST(test_invalid_pattern_ambiguous); + RUN_TEST(test_pattern_matching_group_based_file_fails); RUN_TEST(test_valid_filebased_patterns); RUN_TEST(test_truncate_deletes_existing_files); RUN_TEST(test_filebased_fails_parent_before_t_missing);