Validate CONTCAR before copying to POSCAR to prevent corruption after forced VASP termination (#416)

* Add CONTCAR validation before copying to POSCAR

- Add is_valid_poscar() utility to check if POSCAR/CONTCAR files are valid
  and parseable before copying, preventing empty/truncated files from being
  used after VASP termination

- Update all VASP handlers to validate CONTCAR before copying to POSCAR:
  fexcf, brions, zbrent, eddrmm, edddav, zheev/eddiag, DriftErrorHandler,
  UnconvergedErrorHandler, and StoppedRunHandler

- Add tests for VaspJob.terminate() process group termination using
  os.killpg() to ensure all MPI workers are killed

- Add tests for is_valid_poscar() covering valid, empty, missing,
  malformed, and truncated POSCAR files

* remove dead code #416 (comment)

Author: janosh

src/custodian/vasp/handlers.py

@@ -35,7 +35,7 @@
 from custodian.utils import backup
 from custodian.vasp.interpreter import VaspModder
 from custodian.vasp.io import load_outcar, load_vasprun
-from custodian.vasp.utils import increase_k_point_density
+from custodian.vasp.utils import increase_k_point_density, is_valid_poscar
 
 __author__ = (
     "Shyue Ping Ong, William Davidson Richards, Anubhav Jain, Wei Chen, Stephen Dacek, Andrew Rosen, Janosh Riebesell"
@@ -226,7 +226,8 @@ def correct(self, directory="./"):
             # https://www.vasp.at/forum/viewtopic.php?p=14827
             if self.error_count["fexcf"] == 0:
                 # First see if last ionic configuration is more stable on rerun
-                actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
+                if is_valid_poscar("CONTCAR", directory):
+                    actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
             elif self.error_count["fexcf"] == 1 and vi["INCAR"].get("IBRION", -1) == 1:
                 # Try more stable geometry optimization method
                 actions.append({"dict": "INCAR", "action": {"_set": {"IBRION": 2}}})
@@ -469,7 +470,8 @@ def correct(self, directory="./"):
 
         if "brions" in self.errors:
             # Copy CONTCAR to POSCAR so we do not lose our progress.
-            actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
+            if is_valid_poscar("CONTCAR", directory):
+                actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
 
             # By default, increase POTIM per the VASP error message. But if that does not work,
             # we should try IBRION = 2 since it is less sensitive to POTIM.
@@ -499,8 +501,9 @@ def correct(self, directory="./"):
 
             ediff = vi["INCAR"].get("EDIFF", 1e-4)
 
-            # Copy CONTCAR to POSCAR. This should always be done so we don't lose our progress.
-            actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
+            # Copy CONTCAR to POSCAR so we don't lose our progress.
+            if is_valid_poscar("CONTCAR", directory):
+                actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
 
             # Tighten EDIFF per the VASP warning message. We tighten it by a factor of 10 unless
             # it is > 1e-6 (in which case we set it to 1e-6) or 1e-8 in which case we stop tightening
@@ -548,11 +551,7 @@ def correct(self, directory="./"):
         if "eddrmm" in self.errors:
             # RMM algorithm is not stable for this calculation
             # Copy CONTCAR to POSCAR if CONTCAR has already been populated.
-            try:
-                is_contcar = Poscar.from_file(os.path.join(directory, "CONTCAR"))
-            except Exception:
-                is_contcar = False
-            if is_contcar:
+            if is_valid_poscar("CONTCAR", directory):
                 actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
             if vi["INCAR"].get("ALGO", "Normal").lower() in {"fast", "veryfast"}:
                 actions.append({"dict": "INCAR", "action": {"_set": {"ALGO": "Normal"}}})
@@ -570,11 +569,7 @@ def correct(self, directory="./"):
 
         if "edddav" in self.errors:
             # Copy CONTCAR to POSCAR if CONTCAR has already been populated.
-            try:
-                is_contcar = Poscar.from_file(os.path.join(directory, "CONTCAR"))
-            except Exception:
-                is_contcar = False
-            if is_contcar:
+            if is_valid_poscar("CONTCAR", directory):
                 actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
             if vi["INCAR"].get("ICHARG", 0) < 10:
                 actions.append({"file": "CHGCAR", "action": {"_file_delete": {"mode": "actual"}}})
@@ -619,11 +614,7 @@ def correct(self, directory="./"):
 
         if self.errors & {"zheev", "eddiag"}:
             # Copy CONTCAR to POSCAR if CONTCAR has already been populated.
-            try:
-                is_contcar = Poscar.from_file(os.path.join(directory, "CONTCAR"))
-            except Exception:
-                is_contcar = False
-            if is_contcar:
+            if is_valid_poscar("CONTCAR", directory):
                 actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
             if vi["INCAR"].get("ALGO", "Normal").lower() == "fast":
                 actions.append({"dict": "INCAR", "action": {"_set": {"ALGO": "Normal"}}})
@@ -1057,8 +1048,9 @@ def correct(self, directory="./"):
         incar = vi["INCAR"]
         outcar = load_outcar(os.path.join(directory, "OUTCAR"))
 
-        # Move CONTCAR to POSCAR
-        actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
+        # Move CONTCAR to POSCAR if valid
+        if is_valid_poscar("CONTCAR", directory):
+            actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
 
         # Set PREC to High so ENAUG can be used to control Augmentation Grid Size
         if incar.get("PREC", "Accurate").lower() != "high":
@@ -1239,10 +1231,9 @@ def correct(self, directory="./"):
         elif not v.converged_ionic:
             # Just continue optimizing and let other handlers fix ionic
             # optimizer parameters
-            actions += [
-                {"dict": "INCAR", "action": {"_set": {"IBRION": 1}}},
-                {"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}},
-            ]
+            actions.append({"dict": "INCAR", "action": {"_set": {"IBRION": 1}}})
+            if is_valid_poscar("CONTCAR", directory):
+                actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
 
         if actions:
             vi = VaspInput.from_directory(directory)
@@ -1927,7 +1918,9 @@ def correct(self, directory="./"):
         i = d["Index"]
         name = shutil.make_archive(os.path.join(directory, f"vasp.chk.{i}"), "gztar")
 
-        actions = [{"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}}]
+        actions = []
+        if is_valid_poscar("CONTCAR", directory):
+            actions.append({"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}})
 
         modder = Modder(actions=[FileActions], directory=directory)
         for action in actions:

src/custodian/vasp/utils.py

@@ -2,14 +2,18 @@
 
 from __future__ import annotations
 
+import logging
+import os
 from typing import TYPE_CHECKING, Any
 
 import numpy as np
-from pymatgen.io.vasp.inputs import Kpoints
+from pymatgen.io.vasp.inputs import Kpoints, Poscar
 
 if TYPE_CHECKING:
     from pymatgen.core import Structure
 
+logger = logging.getLogger(__name__)
+
 
 def _estimate_num_k_points_from_kspacing(structure: Structure, kspacing: float) -> tuple[int, ...]:
     """
@@ -103,3 +107,56 @@ def increase_k_point_density(
         mult_fac += factor
 
     return new_kpoints if success else {}  # type: ignore
+
+
+def is_valid_poscar(filename: str, directory: str = "./") -> bool:
+    """Check if a POSCAR/CONTCAR file is valid and can be parsed.
+
+    This is useful to verify CONTCAR is complete before copying to POSCAR,
+    especially after terminating a VASP job which might leave incomplete files.
+
+    Args:
+        filename: Name of the file (e.g., "CONTCAR", "POSCAR")
+        directory: Directory containing the file
+
+    Returns:
+        True if the file exists, is non-empty, and can be parsed as a valid
+        VASP structure file. False otherwise.
+    """
+    filepath = os.path.join(directory, filename)
+
+    # Check file exists
+    if not os.path.isfile(filepath):
+        logger.warning(f"{filename} does not exist in {directory}")
+        return False
+
+    # Check file is not empty
+    if os.path.getsize(filepath) == 0:
+        logger.warning(f"{filename} is empty")
+        return False
+
+    # Try to parse as POSCAR
+    try:
+        Poscar.from_file(filepath)
+        return True
+    except Exception as exc:
+        logger.warning(f"{filename} could not be parsed: {exc}")
+        return False
+
+
+def copy_contcar_to_poscar_if_valid(directory: str = "./") -> list[dict]:
+    """Return action to copy CONTCAR to POSCAR only if CONTCAR is valid.
+
+    This prevents copying incomplete CONTCAR files that may result from
+    terminating VASP mid-write.
+
+    Args:
+        directory: Directory containing CONTCAR
+
+    Returns:
+        List containing the copy action if CONTCAR is valid, empty list otherwise.
+    """
+    if is_valid_poscar("CONTCAR", directory):
+        return [{"file": "CONTCAR", "action": {"_file_copy": {"dest": "POSCAR"}}}]
+    logger.warning("CONTCAR is not valid, skipping copy to POSCAR")
+    return []

tests/vasp/test_jobs.py

@@ -350,5 +350,6 @@ def test_integration_with_real_process(self) -> None:
 
         # Process should be confirmed dead (terminate() waits internally)
         assert real_process.poll() is not None
+        # Verify process group no longer exists
         with pytest.raises(ProcessLookupError):
             os.killpg(original_pgid, 0)

tests/vasp/test_utils.py

@@ -9,7 +9,7 @@
 from pymatgen.io.vasp import Kpoints
 from pymatgen.util.testing import MatSciTest
 
-from custodian.vasp.utils import _estimate_num_k_points_from_kspacing, increase_k_point_density
+from custodian.vasp.utils import _estimate_num_k_points_from_kspacing, increase_k_point_density, is_valid_poscar
 from tests.conftest import TEST_FILES
 
 
@@ -41,3 +41,53 @@ def test_kpoint_density_increase(self):
 
         new_kpoints = increase_k_point_density(Kpoints(), self.small_structure, force_gamma=True, min_kpoints=14)
         assert new_kpoints["kpoints"] == ((3, 3, 3),)
+
+
+class TestIsValidPoscar:
+    """Tests for is_valid_poscar utility function."""
+
+    def test_valid_poscar(self, tmp_path) -> None:
+        """Valid POSCAR file should return True."""
+        poscar_path = tmp_path / "POSCAR"
+        poscar_path.write_text(
+            """Si2
+1.0
+3.8 0.0 0.0
+0.0 3.8 0.0
+0.0 0.0 3.8
+Si
+2
+direct
+0.0 0.0 0.0
+0.5 0.5 0.5
+"""
+        )
+        assert is_valid_poscar("POSCAR", str(tmp_path)) is True
+
+    def test_empty_poscar(self, tmp_path) -> None:
+        """Empty POSCAR file should return False."""
+        poscar_path = tmp_path / "CONTCAR"
+        poscar_path.write_text("")
+        assert is_valid_poscar("CONTCAR", str(tmp_path)) is False
+
+    def test_missing_poscar(self, tmp_path) -> None:
+        """Missing POSCAR file should return False."""
+        assert is_valid_poscar("CONTCAR", str(tmp_path)) is False
+
+    def test_malformed_poscar(self, tmp_path) -> None:
+        """Malformed POSCAR file should return False."""
+        poscar_path = tmp_path / "CONTCAR"
+        poscar_path.write_text("This is not a valid POSCAR file\nGarbage data")
+        assert is_valid_poscar("CONTCAR", str(tmp_path)) is False
+
+    def test_truncated_poscar(self, tmp_path) -> None:
+        """Truncated POSCAR (incomplete write) should return False."""
+        poscar_path = tmp_path / "CONTCAR"
+        # Incomplete POSCAR - cut off mid-file
+        poscar_path.write_text(
+            """Si2
+1.0
+3.8 0.0 0.0
+"""
+        )
+        assert is_valid_poscar("CONTCAR", str(tmp_path)) is False