Overwriting a volume can leave a dangling volume file in the database #136

Activity

1. ebrahimebrahim

   added

   bugSomething isn't workingSomething isn't working

   on Oct 16, 2024
2. ebrahimebrahim

   added

   good first issueGood for newcomersGood for newcomers

   on Mar 3, 2025
3. HussainAther commented on Mar 4, 2025

   

   HussainAther

   on Mar 4, 2025

   More actions

   Hey @ebrahimebrahim, I’m interested in fixing this! Could you clarify the expected behavior—should we delete the old volume immediately or mark it for cleanup?
4. ebrahimebrahim commented on Mar 4, 2025

   

   ebrahimebrahim

   on Mar 4, 2025

   CollaboratorAuthor

   More actions

   This one is a weird edge case, a good way to explain it is perhaps to give you a chunk of code that fails because of it. So here is a unit test that you can add to test_databse to see the issue:

   def test_issue_136(example_database:Database, tmp_path:Path):
       subject_id = "example_subject"
       volume_id = "a_volume_id"
       volume_name = "a_volume_name"
       volume_data_path = Path(tmp_path/'test_db_files/a_volume_filename.nii')
       volume_data_path.parent.mkdir(parents=True, exist_ok=True)
       volume_data_path.touch()
       volume_data_path2 = Path(tmp_path/'test_db_files/a_different_volume_filename.nii')
       volume_data_path2.parent.mkdir(parents=True, exist_ok=True)
       volume_data_path2.touch()
   
       # write volume with the data path volume_data_path
       example_database.write_volume(subject_id, volume_id, volume_name, volume_data_path)
       
       # check that the file is there in the DB with the expected name
       volume_filepath = example_database.get_volume_metadata_filepath(subject_id, volume_id)
       assert((volume_filepath.parent/volume_data_path.name).exists())
   
       # now overwrite using a different volume data path volume_data_path2, with a different filename
       example_database.write_volume(subject_id, volume_id, volume_name, volume_data_path2, on_conflict=OnConflictOpts.OVERWRITE)
   
       # check that the file is there in the DB with the expected name
       volume_filepath = example_database.get_volume_metadata_filepath(subject_id, volume_id)
       assert((volume_filepath.parent/volume_data_path2.name).exists())
   
       # check that the old file was removed
       assert(not (volume_filepath.parent/volume_data_path.name).exists())

   Paste this into test_database.py and run it with pytest -k issue_136, or put it into a notebook. The last line, checking that the old file was removed, is what fails
5. ebrahimebrahim

   assigned

   HussainAther

   on Mar 4, 2025
6. Alexd062024 commented on Sep 22, 2026

   

   Alexd062024

   on Sep 22, 2026

   More actions

   Hi! I reproduced this issue on the current main branch as part of the triage work for #460.

   Commit tested:
   58069a0ffee2e104edc7cca7e9464d70881e89e2

   Environment:

   • Ubuntu 24.04.4 LTS
   • Python 3.12.3
   • openlifu 0.21.2.dev20+g58069a0ff
   • pytest 8.4.2

   Steps to reproduce:

   1. Create a temporary copy of tests/resources/example_db.
   2. Create two existing synthetic files with different names:
      • a_volume_filename.nii
      • a_different_volume_filename.nii
   3. Call Database.write_volume(...) with the first file.
   4. Call Database.write_volume(...) again using the same volume ID, the second file, and on_conflict=OnConflictOpts.OVERWRITE.
   5. Inspect the volume metadata and the volume directory.

   Observed result:

   • data_filename is correctly updated to a_different_volume_filename.nii.
   • get_volume_info(...)["data_abspath"] correctly points to the new file.
   • The new file exists.
   • The previously referenced file, a_volume_filename.nii, still remains in the volume directory.

   The final check:

   assert not old_in_database.exists()

   still fails on the tested main commit.

   I reproduced the same behavior in three independent temporary database copies.

   Expected result:
   After a successful overwrite using a different data filename, the previously referenced volume file should be removed from the volume directory.

   Additional notes:

   • tests/test_database.py passes normally before and after the reproduction (56 passed).
   • No hardware or external medical data was required.
   • The reproduction is aligned with the test case described above in this issue.
   • I also checked Fix stale volume file on overwrite #497, which already contains a proposed fix for this issue. Since it has not been merged yet, the behavior is still reproducible on the current main branch.

   For the #460 triage task, I would classify this issue as:

   REPRODUCED
7. ebrahimebrahim commented on Sep 22, 2026

   

   ebrahimebrahim

   on Sep 22, 2026

   CollaboratorAuthor

   More actions

   Hi @Alexd062024 I believe this issue was already addressed and under review; I am just waiting to ahev the time to review it. Thanks

   👍React with 👍1Reacted by Alexd062024