Note on provenance: This issue was found by Claude Opus 5 while reviewing #320. The reproductions and output shown below were produced by building and running the code, but I have only briefly reviewed the analysis myself and I am not an expert in this area. Please review it critically.
Impact
If Subject initialization fails, there is no way to fix it. The failed attempt has already written most of the subject to the file, and a second attempt with corrected metadata reports success while changing nothing.
The user ends up with an incomplete subject, a file that will not pass DANDI validation, and a return value telling them everything worked. The most likely way to get there today is a date_of_birth that the validator rejects, which #329 shows happens for timestamps that are perfectly valid.
What happens
Two behaviours combine into a dead end.
The failure is not atomic. Subject::initialize (src/nwb/file/Subject.cpp:73) sets initStatus = Status::Failure when date_of_birth does not validate, then keeps going. The group, its type attributes, and every other field are still written.
The retry is ignored. NWBFile::initialize (src/nwb/NWBFile.cpp:106) skips subject creation entirely when /general/subject already exists, logging to std::cerr and leaving initStatus at Success.
Observed behaviour
First call, dateOfBirth = "January 15, 2024":
Subject::initialize returned Failure
group exists, age written, subject_id written, species written, date_of_birth absent
Second call on the same file, with a valid dateOfBirth, species, and sex:
NWBFile::initialize returned Success
date_of_birth absent, species absent, sex absent
Suggested direction
Validate the whole SubjectSpec before anything is written, and return early on a bad value. That keeps a failed initialize from touching the file, which is what makes a retry meaningful.
The retry path then needs to do something other than silently succeed. Either write the fields that are missing, or return Status::Failure so the caller learns the metadata was discarded.
Notes
TEST_CASE("initialize preserves existing subject metadata") in tests/testNWBFile.cpp asserts both halves of this: that the second initialize returns Success, and that the supplied spec was ignored. So the behaviour described above is currently pinned by a test, and changing it means changing that test too. Whether "preserves existing metadata" is the right contract is worth deciding deliberately, since the caller cannot tell it from a successful write.
Impact
If
Subjectinitialization fails, there is no way to fix it. The failed attempt has already written most of the subject to the file, and a second attempt with corrected metadata reports success while changing nothing.The user ends up with an incomplete subject, a file that will not pass DANDI validation, and a return value telling them everything worked. The most likely way to get there today is a
date_of_birththat the validator rejects, which #329 shows happens for timestamps that are perfectly valid.What happens
Two behaviours combine into a dead end.
The failure is not atomic.
Subject::initialize(src/nwb/file/Subject.cpp:73) setsinitStatus = Status::Failurewhendate_of_birthdoes not validate, then keeps going. The group, its type attributes, and every other field are still written.The retry is ignored.
NWBFile::initialize(src/nwb/NWBFile.cpp:106) skips subject creation entirely when/general/subjectalready exists, logging tostd::cerrand leavinginitStatusatSuccess.Observed behaviour
First call,
dateOfBirth = "January 15, 2024":Second call on the same file, with a valid
dateOfBirth,species, andsex:Suggested direction
Validate the whole
SubjectSpecbefore anything is written, and return early on a bad value. That keeps a failedinitializefrom touching the file, which is what makes a retry meaningful.The retry path then needs to do something other than silently succeed. Either write the fields that are missing, or return
Status::Failureso the caller learns the metadata was discarded.Notes
TEST_CASE("initialize preserves existing subject metadata")intests/testNWBFile.cppasserts both halves of this: that the secondinitializereturnsSuccess, and that the supplied spec was ignored. So the behaviour described above is currently pinned by a test, and changing it means changing that test too. Whether "preserves existing metadata" is the right contract is worth deciding deliberately, since the caller cannot tell it from a successful write.