Repository: DataBiosphere/toil

Issue:
Slurm job timeout doesn't seem to register as failure
I just ran into the Slurm time limit for the first time.  From the Toil logs, it looks like my job starts again, with no job failure or error message.  Since there's nothing about deprecating `retryCount`, I suspect it may go on forever.  It's only by looking in the `--batchLogsDir` that I can find a useful message like. 

```
slurmstepd-phoenix-07: error: *** JOB 1449736 ON phoenix-07 CANCELLED AT 2023-12-03T12:47:09 DUE TO TIME LIMIT ***
```

From what I remember, if I have a job killed because of too much memory, even if I need to go into `batchLogs` to see why it was killed, the main Toil log at least shows the failure.  It seems suspicious that this isn't the case here. 

```
2023-12-03 00:47:28.365363: Running the command: "vg index -t 72 -j /data/tmp/c75c5a6e213a5ccdbf30f0ea018ea009/7be9/6287/tmpuoenbd43/clip.t2t-apes-small-pangenome-nosplit-nov30.dist /data/tmp/c75c5a6e213a5ccdbf30f0ea018ea009/7be9/6287/tmpuoenbd43/clip.t2t-apes-small-pangenome-nosplit-nov30.gbz"
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
Issued job 'make_giraffe_indexes' kind-make_giraffe_indexes/instance-8p3w3wnh v1 with job batch system ID: 76 and disk: 381.3 Gi, memory: 1.5 Ti, cores: 72, accelerators: [], preemptible: False
2023-12-03 12:47:31.501711: Running the command: "vg index -t 72 -j /data/tmp/6e792d19fa1c5edbb77567d767580617/fb42/46cf/tmp9zvikqn9/clip.t2t-apes-small-pangenome-nosplit-nov30.dist /data/tmp/6e792d19fa1c5edbb77567d767580617/fb42/46cf/tmp9zvikqn9/clip.t2t-apes-small-pangenome-nosplit-nov30.gbz"
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
1 jobs are running, 0 jobs are issued and waiting to run
Issued job 'make_giraffe_indexes' kind-make_giraffe_indexes/instance-8p3w3wnh v1 with job batch system ID: 77 and disk: 381.3 Gi, memory: 1.5 Ti, cores: 72, accelerators: [], preemptible: False
```

┆Issue is synchronized with this [Jira Story](https://ucsc-cgl.atlassian.net/browse/TOIL-1460)
┆Issue Number: TOIL-1460


Discussion:

Patch:
diff --git a/src/toil/batchSystems/abstractBatchSystem.py b/src/toil/batchSystems/abstractBatchSystem.py
index 9da541de..fd0b30f9 100644
--- a/src/toil/batchSystems/abstractBatchSystem.py
+++ b/src/toil/batchSystems/abstractBatchSystem.py
@@ -59,13 +59,28 @@ class BatchJobExitReason(enum.IntEnum):
     MEMLIMIT: int = 6
     """Job hit batch system imposed memory limit."""
 
+    @classmethod
+    def to_string(cls, value: int) -> str:
+        """
+        Convert to human-readable string.
+
+        Given an int that may be or may be equal to a value from the enum,
+        produce the string value of its matching enum entry, or a stringified
+        int.
+        """
+        try:
+            return cls(value).name
+        except ValueError:
+            return str(value)
+
 class UpdatedBatchJobInfo(NamedTuple):
     jobID: int
     exitStatus: int
     """
     The exit status (integer value) of the job. 0 implies successful.
 
-    EXIT_STATUS_UNAVAILABLE_VALUE is used when the exit status is not available (e.g. job is lost).
+    EXIT_STATUS_UNAVAILABLE_VALUE is used when the exit status is not available
+    (e.g. job is lost, or otherwise died but actual exit code was not reported).
     """
 
     exitReason: Optional[BatchJobExitReason]
@@ -437,7 +452,7 @@ class BatchSystemSupport(AbstractBatchSystem):
         file_name: str = f'toil_{self.config.workflowID}.{toil_job_id}.{cluster_job_id}.{std}.log'
         logs_dir: str = self.get_batch_logs_dir()
         return os.path.join(logs_dir, file_name)
-    
+
     def format_std_out_err_glob(self, toil_job_id: int) -> str:
         """
         Get a glob string that will match all file paths generated by format_std_out_err_path for a job.
@@ -445,7 +460,7 @@ class BatchSystemSupport(AbstractBatchSystem):
         file_glob: str = f'toil_{self.config.workflowID}.{toil_job_id}.*.log'
         logs_dir: str = self.get_batch_logs_dir()
         return os.path.join(logs_dir, file_glob)
-        
+
     @staticmethod
     def workerCleanup(info: WorkerCleanupInfo) -> None:
         """
@@ -509,7 +524,7 @@ class AbstractScalableBatchSystem(AbstractBatchSystem):
     """
     A batch system that supports a variable number of worker nodes.
 
-    Used by :class:`toil.provisioners.clusterScaler.ClusterScaler` 
+    Used by :class:`toil.provisioners.clusterScaler.ClusterScaler`
     to scale the number of worker nodes in the cluster
     up or down depending on overall load.
     """
diff --git a/src/toil/batchSystems/abstractGridEngineBatchSystem.py b/src/toil/batchSystems/abstractGridEngineBatchSystem.py
index 16dd563a..93986381 100644
--- a/src/toil/batchSystems/abstractGridEngineBatchSystem.py
+++ b/src/toil/batchSystems/abstractGridEngineBatchSystem.py
@@ -225,23 +225,20 @@ class AbstractGridEngineBatchSystem(BatchSystemCleanupSupport):
             return activity
 
         def _handle_job_status(
-            self, job_id: int, status: Union[int, None], activity: bool
+            self, job_id: int, status: Union[int, Tuple[int, Optional[BatchJobExitReason]], None], activity: bool
         ) -> bool:
             """
             Helper method for checkOnJobs to handle job statuses
             """
             if status is not None:
+                if isinstance(status, int):
+                    code = status
+                    reason = None
+                else:
+                    code, reason = status
                 self.updatedJobsQueue.put(
                     UpdatedBatchJobInfo(
-                        jobID=job_id, exitStatus=status, exitReason=None, wallTime=None
-                    )
-                )
-                self.forgetJob(job_id)
-                return True
-            if status is not None and isinstance(status, BatchJobExitReason):
-                self.updatedJobsQueue.put(
-                    UpdatedBatchJobInfo(
-                        jobID=job_id, exitStatus=1, exitReason=status, wallTime=None
+                        jobID=job_id, exitStatus=code, exitReason=reason, wallTime=None
                     )
                 )
                 self.forgetJob(job_id)
@@ -279,9 +276,9 @@ class AbstractGridEngineBatchSystem(BatchSystemCleanupSupport):
                 logger.error("GridEngine like batch system failure", exc_info=ex)
                 raise
 
-        def coalesce_job_exit_codes(self, batch_job_id_list: list) -> list:
+        def coalesce_job_exit_codes(self, batch_job_id_list: list) -> List[Union[int, Tuple[int, Optional[BatchJobExitReason]], None]]:
             """
-            Returns exit codes for a list of jobs.
+            Returns exit codes and possibly exit reasons for a list of jobs, or None if they are running.
 
             Called by AbstractGridEngineWorker.checkOnJobs().
 
@@ -351,16 +348,19 @@ class AbstractGridEngineBatchSystem(BatchSystemCleanupSupport):
             raise NotImplementedError()
 
         @abstractmethod
-        def getJobExitCode(self, batchJobID):
+        def getJobExitCode(self, batchJobID) -> Union[int, Tuple[int, Optional[BatchJobExitReason]], None]:
             """
-            Returns job exit code or an instance of abstractBatchSystem.BatchJobExitReason.
-            if something else happened other than the job exiting.
-            Implementation-specific; called by AbstractGridEngineWorker.checkOnJobs()
+            Returns job exit code and possibly an instance of abstractBatchSystem.BatchJobExitReason.
 
-            :param string batchjobID: batch system job ID
+            Returns None if the job is still running.
+
+            If the job is not running but the exit code is not available, it
+            will be EXIT_STATUS_UNAVAILABLE_VALUE. Implementation-specific;
+            called by AbstractGridEngineWorker.checkOnJobs().
 
-            :rtype: int|toil.batchSystems.abstractBatchSystem.BatchJobExitReason: exit code int
-                    or BatchJobExitReason if something else happened other than job exiting.
+            The exit code will only be 0 if the job affirmatively succeeded.
+
+            :param string batchjobID: batch system job ID
             """
             raise NotImplementedError()
 
diff --git a/src/toil/batchSystems/lsf.py b/src/toil/batchSystems/lsf.py
index 645e54db..68dcef7a 100644
--- a/src/toil/batchSystems/lsf.py
+++ b/src/toil/batchSystems/lsf.py
@@ -25,12 +25,12 @@ import re
 import subprocess
 from datetime import datetime
 from random import randint
-from typing import Dict, List, Optional, Union
+from typing import Dict, List, Optional, Tuple, Union
 
 from dateutil.parser import parse
 from dateutil.tz import tzlocal
 
-from toil.batchSystems.abstractBatchSystem import BatchJobExitReason
+from toil.batchSystems.abstractBatchSystem import BatchJobExitReason, EXIT_STATUS_UNAVAILABLE_VALUE
 from toil.batchSystems.abstractGridEngineBatchSystem import \
     AbstractGridEngineBatchSystem
 from toil.batchSystems.lsfHelper import (check_lsf_json_output_supported,
@@ -161,7 +161,7 @@ class LSFBatchSystem(AbstractGridEngineBatchSystem):
                         status_resonse.append(None)
             return status_resonse
 
-        def getJobExitCode(self, lsfJobID):
+        def getJobExitCode(self, lsfJobID) -> Union[int, Tuple[int, Optional[BatchJobExitReason]], None]:
             # the task is set as part of the job ID if using getBatchSystemID()
             if "NOT_SUBMITTED" in lsfJobID:
                 logger.error("bjobs detected job failed to submit")
@@ -186,7 +186,7 @@ class LSFBatchSystem(AbstractGridEngineBatchSystem):
 
             return self.fallbackGetJobExitCode(job)
 
-        def parse_bjobs_record(self, bjobs_record: dict, job: int) -> Union[int, None]:
+        def parse_bjobs_record(self, bjobs_record: dict, job: int) -> Union[int, Tuple[int, Optional[BatchJobExitReason]], None]:
             """
             Helper functions for getJobExitCode and  to parse the bjobs status record
             """
@@ -224,7 +224,7 @@ class LSFBatchSystem(AbstractGridEngineBatchSystem):
                         exit_info, job
                     )
                     if "TERM_MEMLIMIT" in exit_reason:
-                        return BatchJobExitReason.MEMLIMIT
+                        return (exit_code if exit_code != 0 else EXIT_STATUS_UNAVAILABLE_VALUE, BatchJobExitReason.MEMLIMIT)
                     return exit_code
                 if process_status == "RUN":
                     logger.debug(
@@ -237,7 +237,7 @@ class LSFBatchSystem(AbstractGridEngineBatchSystem):
 
                 return self.getJobExitCodeBACCT(job)
 
-        def getJobExitCodeBACCT(self,job):
+        def getJobExitCodeBACCT(self,job) -> Union[int, Tuple[int, Optional[BatchJobExitReason]], None]:
             # if not found in bjobs, then try bacct (slower than bjobs)
             logger.debug("bjobs failed to detect job - trying bacct: "
                          "{}".format(job))
@@ -258,7 +258,7 @@ class LSFBatchSystem(AbstractGridEngineBatchSystem):
                          "running: {}".format(job))
             return None
 
-        def fallbackGetJobExitCode(self, job):
+        def fallbackGetJobExitCode(self, job) -> Union[int, Tuple[int, Optional[BatchJobExitReason]], None]:
             args = ["bjobs", "-l", str(job)]
             logger.debug(f"Checking job exit code for job via bjobs (fallback): {job}")
             stdout = call_command(args)
diff --git a/src/toil/batchSystems/slurm.py b/src/toil/batchSystems/slurm.py
index fc4c4941..6abdacb1 100644
--- a/src/toil/batchSystems/slurm.py
+++ b/src/toil/batchSystems/slurm.py
@@ -16,8 +16,9 @@ import math
 import os
 from argparse import ArgumentParser, _ArgumentGroup
 from shlex import quote
-from typing import Dict, List, Optional, TypeVar, Union
+from typing import Dict, List, Optional, Tuple, TypeVar, Union
 
+from toil.batchSystems.abstractBatchSystem import BatchJobExitReason, EXIT_STATUS_UNAVAILABLE_VALUE
 from toil.batchSystems.abstractGridEngineBatchSystem import \
     AbstractGridEngineBatchSystem
 from toil.batchSystems.options import OptionSetter
@@ -97,12 +98,12 @@ class SlurmBatchSystem(AbstractGridEngineBatchSystem):
                 logger.error("sbatch command failed")
                 raise e
 
-        def coalesce_job_exit_codes(self, batch_job_id_list: list) -> list:
+        def coalesce_job_exit_codes(self, batch_job_id_list: list) -> List[Union[int, Tuple[int, Optional[BatchJobExitReason]], None]]:
             """
             Collect all job exit codes in a single call.
             :param batch_job_id_list: list of Job ID strings, where each string has the form
             "<job>[.<task>]".
-            :return: list of job exit codes, associated with the list of job IDs.
+            :return: list of job exit codes or exit code, exit reason pairs associated with the list of job IDs.
             """
             logger.debug("Getting exit codes for slurm jobs: %s", batch_job_id_list)
             # Convert batch_job_id_list to list of integer job IDs.
@@ -113,7 +114,7 @@ class SlurmBatchSystem(AbstractGridEngineBatchSystem):
                 exit_codes.append(self._get_job_return_code(status))
             return exit_codes
 
-        def getJobExitCode(self, batchJobID: str) -> int:
+        def getJobExitCode(self, batchJobID: str) -> Union[int, Tuple[int, Optional[BatchJobExitReason]], None]:
             """
             Get job exit code for given batch job ID.
             :param batchJobID: string of the form "<job>[.<task>]".
@@ -140,18 +141,68 @@ class SlurmBatchSystem(AbstractGridEngineBatchSystem):
                 status_dict = self._getJobDetailsFromScontrol(job_id_list)
             return status_dict
 
-        def _get_job_return_code(self, status: tuple) -> list:
+        def _get_job_return_code(self, status: tuple) -> Union[int, Tuple[int, Optional[BatchJobExitReason]], None]:
             """
+            Given a Slurm return code, status pair, summarize them into a Toil return code, exit reason pair.
+
+            The return code may have already been OR'd with the 128-offset
+            Slurm-reported signal.
+
+            Slurm will report return codes of 0 even if jobs time out instead
+            of succeeding:
+
+                2093597|TIMEOUT|0:0
+                2093597.batch|CANCELLED|0:15
+
+            So we guarantee here that, if the Slurm status string is not a
+            successful one as defined in
+            <https://slurm.schedmd.com/squeue.html#SECTION_JOB-STATE-CODES>, we
+            will not return a successful return code.
+
             Helper function for `getJobExitCode` and `coalesce_job_exit_codes`.
-            :param status: tuple containing the job's state and it's return code.
-            :return: the job's return code if it's completed, otherwise None.
+            :param status: tuple containing the job's state and it's return code from Slurm.
+            :return: the job's return code for Toil if it's completed, otherwise None.
             """
             state, rc = status
-            # If job is in a running state, set return code to None to indicate we don't have
-            # an update.
-            if state in ('PENDING', 'RUNNING', 'CONFIGURING', 'COMPLETING', 'RESIZING', 'SUSPENDED'):
-                rc = None
-            return rc
+
+            # If a job is in one of these states, Slurm can't run it anymore.
+            # We don't include states where the job is held or paused here;
+            # those mean it could run and needs to wait for someone to un-hold
+            # it, so Toil should wait for it.
+            #
+            # We map from each terminal state to the Toil-ontology exit reason.
+            TERMINAL_STATES: Dict[str, BatchJobExitReason] = {
+                "BOOT_FAIL": BatchJobExitReason.LOST,
+                "CANCELLED": BatchJobExitReason.KILLED,
+                "COMPLETED": BatchJobExitReason.FINISHED,
+                "DEADLINE": BatchJobExitReason.KILLED,
+                "FAILED": BatchJobExitReason.FAILED,
+                "NODE_FAIL": BatchJobExitReason.LOST,
+                "OUT_OF_MEMORY": BatchJobExitReason.MEMLIMIT,
+                "PREEMPTED": BatchJobExitReason.KILLED,
+                "TIMEOUT": BatchJobExitReason.KILLED
+            }
+
+            if state not in TERMINAL_STATES:
+                # Don't treat the job as exited yet
+                return None
+
+            exit_reason = TERMINAL_STATES[state]
+
+            if exit_reason == BatchJobExitReason.FINISHED:
+                # The only state that should produce a 0 ever is COMPLETED. So
+                # if the job is COMPLETED and the exit reason is thus FINISHED,
+                # pass along the code it has.
+                return (rc, exit_reason)
+
+            if rc == 0:
+                # The job claims to be in a state other than COMPLETED, but
+                # also to have not encountered a problem. Say the exit status
+                # is unavailable.
+                return (EXIT_STATUS_UNAVAILABLE_VALUE, exit_reason)
+
+            # If the code is nonzero, pass it along.
+            return (rc, exit_reason)
 
         def _getJobDetailsFromSacct(self, job_id_list: list) -> dict:
             """
diff --git a/src/toil/leader.py b/src/toil/leader.py
index 97a069b8..892cb3ad 100644
--- a/src/toil/leader.py
+++ b/src/toil/leader.py
@@ -28,7 +28,8 @@ import enlighten
 from toil import resolveEntryPoint
 from toil.batchSystems import DeadlockException
 from toil.batchSystems.abstractBatchSystem import (AbstractBatchSystem,
-                                                   BatchJobExitReason)
+                                                   BatchJobExitReason,
+                                                   EXIT_STATUS_UNAVAILABLE_VALUE)
 from toil.bus import (JobCompletedMessage,
                       JobFailedMessage,
                       JobIssuedMessage,
@@ -706,8 +707,9 @@ class Leader:
             if exitStatus == 0:
                 logger.debug('Job ended: %s', updatedJob)
             else:
-                logger.warning(f'Job failed with exit value {exitStatus}: {updatedJob}\n'
-                               f'Exit reason: {exitReason}')
+                status_string = str(exitStatus) if exitStatus != EXIT_STATUS_UNAVAILABLE_VALUE else "<UNAVAILABLE>"
+                logger.warning(f'Job failed with exit value {status_string}: {updatedJob}\n'
+                               f'Exit reason: {BatchJobExitReason.to_string(exitReason)}')
                 if exitStatus == CWL_UNSUPPORTED_REQUIREMENT_EXIT_CODE:
                     # This is a CWL job informing us that the workflow is
                     # asking things of us that Toil can't do. When we raise an


Test patch:
diff --git a/src/toil/test/batchSystems/test_slurm.py b/src/toil/test/batchSystems/test_slurm.py
index d8b3ac32..0e68391f 100644
--- a/src/toil/test/batchSystems/test_slurm.py
+++ b/src/toil/test/batchSystems/test_slurm.py
@@ -4,6 +4,7 @@ from queue import Queue
 import pytest
 
 import toil.batchSystems.slurm
+from toil.batchSystems.abstractBatchSystem import BatchJobExitReason, EXIT_STATUS_UNAVAILABLE_VALUE
 from toil.common import Config
 from toil.lib.misc import CalledProcessErrorStderr
 from toil.test import ToilTest
@@ -284,7 +285,7 @@ class SlurmTest(ToilTest):
     def test_getJobExitCode_job_exists(self):
         self.monkeypatch.setattr(toil.batchSystems.slurm, "call_command", call_sacct)
         job_id = '785023'  # FAILED
-        expected_result = 127
+        expected_result = (127, BatchJobExitReason.FAILED)
         result = self.worker.getJobExitCode(job_id)
         assert result == expected_result, f"{result} != {expected_result}"
 
@@ -303,7 +304,7 @@ class SlurmTest(ToilTest):
         self.monkeypatch.setattr(self.worker, "_getJobDetailsFromSacct", call_sacct_raises)
         self.monkeypatch.setattr(toil.batchSystems.slurm, "call_command", call_scontrol)
         job_id = '787204'  # COMPLETED
-        expected_result = 0
+        expected_result = (0,  BatchJobExitReason.FINISHED)
         result = self.worker.getJobExitCode(job_id)
         assert result == expected_result, f"{result} != {expected_result}"
 
@@ -329,7 +330,7 @@ class SlurmTest(ToilTest):
     def test_coalesce_job_exit_codes_one_exists(self):
         self.monkeypatch.setattr(toil.batchSystems.slurm, "call_command", call_sacct)
         job_ids = ['785023']  # FAILED
-        expected_result = [127]
+        expected_result = [(127,  BatchJobExitReason.FAILED)]
         result = self.worker.coalesce_job_exit_codes(job_ids)
         assert result == expected_result, f"{result} != {expected_result}"
 
@@ -347,7 +348,14 @@ class SlurmTest(ToilTest):
                    '789724',  # RUNNING,
                    '789868',  # PENDING,
                    '789869']  # COMPLETED
-        expected_result = [0, 1, None, None, 0]  # RUNNING and PENDING jobs should return None
+        # RUNNING and PENDING jobs should return None
+        expected_result = [
+            (EXIT_STATUS_UNAVAILABLE_VALUE, BatchJobExitReason.KILLED),
+            (1, BatchJobExitReason.FAILED),
+            None,
+            None,
+            (0, BatchJobExitReason.FINISHED)
+        ]
         result = self.worker.coalesce_job_exit_codes(job_ids)
         assert result == expected_result, f"{result} != {expected_result}"
 
@@ -358,7 +366,14 @@ class SlurmTest(ToilTest):
                    '789724',  # RUNNING,
                    '999999',  # Non-existent,
                    '789869']  # COMPLETED
-        expected_result = [130, 2, None, None, 0]    # RUNNING job should return None
+        # RUNNING job should return None
+        expected_result = [
+            (130, BatchJobExitReason.FAILED),
+            (2, BatchJobExitReason.FAILED),
+            None,
+            None,
+            (0, BatchJobExitReason.FINISHED)
+        ]
         result = self.worker.coalesce_job_exit_codes(job_ids)
         assert result == expected_result, f"{result} != {expected_result}"
 
@@ -370,7 +385,7 @@ class SlurmTest(ToilTest):
         self.monkeypatch.setattr(self.worker, "_getJobDetailsFromSacct", call_sacct_raises)
         self.monkeypatch.setattr(toil.batchSystems.slurm, "call_command", call_scontrol)
         job_ids = ['787204']  # COMPLETED
-        expected_result = [0]
+        expected_result = [(0, BatchJobExitReason.FINISHED)]
         result = self.worker.coalesce_job_exit_codes(job_ids)
         assert result == expected_result, f"{result} != {expected_result}"