Repository: glotzerlab/signac
Pull request #467 (2021-01-07 20:01:13)

Fix: don't delete invalid statepoints
## Description
The (unreleased) lazy loading code recently introduced in #239 and/or #451 appears to delete invalid state point files if the state point on disk is invalid AND `job.init()` is called before `job.statepoint`. In my understanding, the only case where a state point file should be deleted is if the initial file creation/write operation fails during `job.init()`.

## Types of Changes
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [ ] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added all related issue and pull request numbers for future reference (if applicable). See example below.

<!-- Example for a changelog entry: `Fix issue with launching rockets to the moon (#101, #212).` -->


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/467?src=pr&el=h1) Report
> Merging [#467](https://codecov.io/gh/glotzerlab/signac/pull/467?src=pr&el=desc) (fcda106) into [feature/lazy-job-init](https://codecov.io/gh/glotzerlab/signac/commit/ee6a0aacbfa94b58994d4df8e1f4c673861101cb?el=desc) (ee6a0aa) will **decrease** coverage by `0.10%`.
> The diff coverage is `76.92%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/467/graphs/tree.svg?width=650&height=150&src=pr&token=FghbjTyzgM)](https://codecov.io/gh/glotzerlab/signac/pull/467?src=pr&el=tree)

```diff
@@                    Coverage Diff                    @@
##           feature/lazy-job-init     #467      +/-   ##
=========================================================
- Coverage                  77.13%   77.02%   -0.11%     
=========================================================
  Files                         42       42              
  Lines                       5751     5755       +4     
  Branches                    1123     1125       +2     
=========================================================
- Hits                        4436     4433       -3     
- Misses                      1028     1034       +6     
- Partials                     287      288       +1     
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/467?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/job.py](https://codecov.io/gh/glotzerlab/signac/pull/467/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvam9iLnB5) | `88.96% <76.92%> (-2.37%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/467?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/467?src=pr&el=footer). Last update [ee6a0aa...fcda106](https://codecov.io/gh/glotzerlab/signac/pull/467?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

bdice: > 
> 
> I'm confused what behavior is actually affected by this PR, besides setting the state point in `_check_manifest`. How do the changes stop corrupt jobs from being deleted if the state point file is invalid?

@tcmoore3 The specific case that caused problems was introduced in previous PRs that added lazy loading. The crucial change is here:
https://github.com/glotzerlab/signac/blob/09a6677a963185634d1060b9495f6001bdadf4d7/signac/contrib/job.py#L529-L534

The previous code (before lazy loading) would already know the state point (it wasn't loaded lazily) and thus could not raise a `JobsCorruptedError` exception here if the data was invalid. However, any exception that _did_ get raised during a dump of the state point to disk was assumed to be an issue with file creation/saving (probably an `OSError`) and would trigger a rollback (deletion of the state point file). The change in this PR is to catch any `JobsCorruptedError` that is raised by calling `self.statepoint()` if the state point was loaded lazily _during `init()`_ but is corrupted on disk. That case should _not_ trigger rollback (file deletion), because we want to grant the user an opportunity to fix the file on disk if the state point is corrupt but not beyond manual repair (whatever that manual repair should look like is partially up to the user and is not within signac's scope -- the important thing is that signac shouldn't just delete the file when raising an error about its invalidity).

The test that I added also provides an example case of what behavior to expect when data is invalid. This fix is simply an effort to provide additional safety when handling invalid states and it is not a case that should arise in normal use.
tcmoore3: @bdice I see. This looks good.

Patch:
diff --git a/signac/contrib/job.py b/signac/contrib/job.py
index 264563fa..812f3947 100644
--- a/signac/contrib/job.py
+++ b/signac/contrib/job.py
@@ -337,10 +337,9 @@ class Job:
 
         """
         if self._statepoint is None:
-            # Load state point manifest lazily
+            # Load state point manifest lazily and assign to
+            # self._statepoint
             statepoint = self._check_manifest()
-            self._statepoint = SyncedAttrDict(statepoint, parent=_sp_save_hook(self))
-
             # Update the project's state point cache when loaded lazily
             self._project._register(self.id, statepoint)
 
@@ -510,35 +509,37 @@ class Job:
             (Default value = False).
 
         """
-        fn_manifest = os.path.join(self.workspace(), self.FN_MANIFEST)
-
         # Attempt early exit if the manifest exists and is valid
         try:
             statepoint = self._check_manifest()
         except Exception:
             # Any exception means this method cannot exit early.
 
-            # Create the workspace directory if it did not exist yet.
+            # Create the workspace directory if it does not exist.
             try:
                 _mkdir_p(self.workspace())
             except OSError:
                 logger.error(
                     "Error occurred while trying to create "
-                    "workspace directory for job '{}'.".format(self)
+                    "workspace directory for job '{}'.".format(self.id)
                 )
                 raise
 
+            fn_manifest = os.path.join(self.workspace(), self.FN_MANIFEST)
             try:
-                # Prepare the data before file creation and writing
-                blob = json.dumps(self.statepoint, indent=2)
+                # Prepare the data before file creation and writing.
+                statepoint = self.statepoint()
+                blob = json.dumps(statepoint, indent=2)
+            except JobsCorruptedError:
+                raise
 
-                try:
-                    # Open the file for writing only if it does not exist yet.
-                    with open(fn_manifest, "w" if force else "x") as file:
-                        file.write(blob)
-                except OSError as error:
-                    if error.errno not in (errno.EEXIST, errno.EACCES):
-                        raise
+            try:
+                # Open the file for writing only if it does not exist yet.
+                with open(fn_manifest, "w" if force else "x") as file:
+                    file.write(blob)
+            except OSError as error:
+                if error.errno not in (errno.EEXIST, errno.EACCES):
+                    raise
             except Exception as error:
                 # Attempt to delete the file on error, to prevent corruption.
                 try:
@@ -547,6 +548,7 @@ class Job:
                     pass
                 raise error
             else:
+                # Validate the output again after writing to disk
                 statepoint = self._check_manifest()
 
         # Update the project's state point cache if the manifest is valid
@@ -555,6 +557,9 @@ class Job:
     def _check_manifest(self):
         """Check whether the manifest file exists and is correct.
 
+        If the manifest is valid, this sets the state point if it is not
+        already set.
+
         Returns
         -------
         manifest : dict
@@ -569,6 +574,8 @@ class Job:
         manifest = self._read_manifest()
         if calc_id(manifest) != self.id:
             raise JobsCorruptedError([self.id])
+        if self._statepoint is None:
+            self._statepoint = SyncedAttrDict(manifest, parent=_sp_save_hook(self))
         return manifest
 
     def init(self, force=False):


Test patch:
diff --git a/tests/test_job.py b/tests/test_job.py
index ff7ed3b9..f083f28e 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -18,12 +18,7 @@ import signac.common.config
 import signac.contrib
 from signac import Project  # noqa: F401
 from signac.contrib.job import Job  # noqa: F401
-from signac.errors import (
-    DestinationExistsError,
-    InvalidKeyError,
-    JobsCorruptedError,
-    KeyTypeError,
-)
+from signac.errors import DestinationExistsError, InvalidKeyError, KeyTypeError
 
 try:
     import h5py  # noqa
@@ -632,8 +627,8 @@ class TestJobOpenAndClosing(TestJobBase):
         job2 = self.open_job(test_token)
         try:
             logging.disable(logging.ERROR)
-            with pytest.raises(JobsCorruptedError):
-                job2.init()
+            # Detects the corrupted manifest and overwrites with valid data
+            job2.init()
         finally:
             logging.disable(logging.NOTSET)
         job2.init(force=True)
diff --git a/tests/test_project.py b/tests/test_project.py
index a0314735..070b00ca 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -482,8 +482,14 @@ class TestProject(TestProjectBase):
                 # Accessing the job state point triggers validation of the
                 # state point manifest file
                 self.project.open_job(id=job.id).statepoint
+            with pytest.raises(JobsCorruptedError):
+                # Initializing the job state point triggers validation of the
+                # state point manifest file
+                self.project.open_job(id=job.id).init()
         finally:
             logging.disable(logging.NOTSET)
+        # Ensure that the corrupted manifest still exists
+        assert os.path.exists(job.fn(job.FN_MANIFEST))
 
     def test_rename_workspace(self):
         job = self.project.open_job(dict(a=0))