From f4a6bb397a1ae6bf2b39bfb31e27d320e7898e81 Mon Sep 17 00:00:00 2001 From: Pranav Date: Sun, 25 Oct 2026 15:41:17 +0530 Subject: [PATCH 1/3] fix: sub-minute job timeouts reported as 0 minutes (closes #9527) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a job's timeout is less than 60 seconds, using integer division imeout // 60 produced 0, causing misleading messages like: 'Job foo timed out after 0 minutes' Introduce _format_timeout(seconds) that formats durations using hours, minutes, and seconds — falling back to seconds when the duration is sub-minute. Replace all three imeout // 60 uses in job_runner.py. Add parametrized tests covering sub-minute, minute-boundary, and multi-hour durations, plus a regression test that asserts '0 minutes' never appears for any timeout in [1, 59] seconds. --- packages/syft-job/src/syft_job/job_runner.py | 33 ++++++++++++++++-- .../syft-job/tests/test_job_runner_timeout.py | 34 ++++++++++++++++++- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/packages/syft-job/src/syft_job/job_runner.py b/packages/syft-job/src/syft_job/job_runner.py index baa50d28f64..0a281f78b23 100644 --- a/packages/syft-job/src/syft_job/job_runner.py +++ b/packages/syft-job/src/syft_job/job_runner.py @@ -48,6 +48,33 @@ def get_job_timeout_seconds() -> int: IS_IN_JOB_ENV_VAR = "SYFT_IS_IN_JOB" + +def _format_timeout(seconds: int) -> str: + """Return a human-readable duration string for a timeout value. + + Sub-minute timeouts are expressed in seconds so they are never reported + as "0 minutes" (issue #9527). + + Examples: + >>> _format_timeout(30) + '30 seconds' + >>> _format_timeout(90) + '1 minute 30 seconds' + >>> _format_timeout(600) + '10 minutes' + """ + minutes, secs = divmod(int(seconds), 60) + hours, minutes = divmod(minutes, 60) + + parts = [] + if hours > 0: + parts.append(f"{hours} hour{'s' if hours != 1 else ''}") + if minutes > 0: + parts.append(f"{minutes} minute{'s' if minutes != 1 else ''}") + if secs > 0 or not parts: + parts.append(f"{secs} second{'s' if secs != 1 else ''}") + return " ".join(parts) + # A job runs from a fresh copy of its approved submission in the system temp # folder, outside the SyftBox: sync never reads it, and no submitter writes it. RUN_DIR_PREFIX = "syft-job-run-" @@ -313,7 +340,7 @@ def _execute_job_streaming(self, ref: JobRef, timeout: int, run_dir: Path) -> in _kill_process_tree(process.pid) process.wait() timed_out = True - print(f" Job {job_name} timed out after {timeout // 60} minutes") + print(f" Job {job_name} timed out after {_format_timeout(timeout)}") stdout_f.write("\n--- PROCESS TIMED OUT ---\n") stderr_f.write("\n--- PROCESS TIMED OUT ---\n") break @@ -384,7 +411,7 @@ def _execute_job_captured(self, ref: JobRef, timeout: int, run_dir: Path) -> int returncode = -1 stdout = (stdout or "") + "\n--- PROCESS TIMED OUT ---\n" stderr = (stderr or "") + "\n--- PROCESS TIMED OUT ---\n" - print(f" Job {job_name} timed out after {timeout // 60} minutes") + print(f" Job {job_name} timed out after {_format_timeout(timeout)}") staging_dir = self.manager.staging_dir(ref) staging_dir.mkdir(parents=True, exist_ok=True) @@ -520,7 +547,7 @@ def _run_approved_job( self._report_result(ref, returncode) except subprocess.TimeoutExpired: - print(f" Job {job_name} timed out after {timeout // 60} minutes") + print(f" Job {job_name} timed out after {_format_timeout(timeout)}") self._finalize(ref, -1) except Exception as e: print(f" Error executing job {job_name}: {e}") diff --git a/packages/syft-job/tests/test_job_runner_timeout.py b/packages/syft-job/tests/test_job_runner_timeout.py index 1092fced969..4823865d759 100644 --- a/packages/syft-job/tests/test_job_runner_timeout.py +++ b/packages/syft-job/tests/test_job_runner_timeout.py @@ -1,6 +1,7 @@ -import pytest +import pytest from syft_job.job_runner import ( DEFAULT_JOB_TIMEOUT_SECONDS, + _format_timeout, get_job_timeout_seconds, ) @@ -35,3 +36,34 @@ def test_job_timeout_rejects_invalid_environment_value( ), ): get_job_timeout_seconds() + + +@pytest.mark.parametrize( + "seconds,expected", + [ + (30, "30 seconds"), + (1, "1 second"), + (60, "1 minute"), + (90, "1 minute 30 seconds"), + (120, "2 minutes"), + (600, "10 minutes"), + (3600, "1 hour"), + (3661, "1 hour 1 minute 1 second"), + (7384, "2 hours 3 minutes 4 seconds"), + ], +) +def test_format_timeout_produces_human_readable_string( + seconds: int, expected: str +) -> None: + """Sub-minute timeouts must not be reported as '0 minutes' (issue #9527).""" + assert _format_timeout(seconds) == expected + + +def test_format_timeout_never_reports_zero_minutes_for_sub_minute_timeout() -> None: + """Regression test for issue #9527: sub-minute timeouts reported as 0 minutes.""" + for seconds in range(1, 60): + result = _format_timeout(seconds) + assert "0 minutes" not in result, ( + f"_format_timeout({seconds}) returned {result!r}, " + "which incorrectly contains '0 minutes'" + ) \ No newline at end of file From e37748cb61b663ed433b5e73a1344cb599e8fc0d Mon Sep 17 00:00:00 2001 From: Pranav V Rao Date: Sun, 25 Oct 2026 16:39:27 +0530 Subject: [PATCH 2/3] test: clean up timeout formatter regression tests --- packages/syft-job/tests/test_job_runner_timeout.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/syft-job/tests/test_job_runner_timeout.py b/packages/syft-job/tests/test_job_runner_timeout.py index 4823865d759..77db7ab5e3a 100644 --- a/packages/syft-job/tests/test_job_runner_timeout.py +++ b/packages/syft-job/tests/test_job_runner_timeout.py @@ -1,4 +1,4 @@ -import pytest +import pytest from syft_job.job_runner import ( DEFAULT_JOB_TIMEOUT_SECONDS, _format_timeout, @@ -41,6 +41,7 @@ def test_job_timeout_rejects_invalid_environment_value( @pytest.mark.parametrize( "seconds,expected", [ + (0, "0 seconds"), (30, "30 seconds"), (1, "1 second"), (60, "1 minute"), @@ -55,7 +56,7 @@ def test_job_timeout_rejects_invalid_environment_value( def test_format_timeout_produces_human_readable_string( seconds: int, expected: str ) -> None: - """Sub-minute timeouts must not be reported as '0 minutes' (issue #9527).""" + """Timeouts should be formatted as readable hours, minutes, and seconds.""" assert _format_timeout(seconds) == expected @@ -66,4 +67,4 @@ def test_format_timeout_never_reports_zero_minutes_for_sub_minute_timeout() -> N assert "0 minutes" not in result, ( f"_format_timeout({seconds}) returned {result!r}, " "which incorrectly contains '0 minutes'" - ) \ No newline at end of file + ) From f1c0f4843b3d597b61e0c6d7f0ac52ca336504f2 Mon Sep 17 00:00:00 2001 From: Koen van der Veen Date: Mon, 26 Oct 2026 11:01:10 +0100 Subject: [PATCH 3/3] test: trim timeout formatter tests to a single parametrized case --- packages/syft-job/src/syft_job/job_runner.py | 1 + .../syft-job/tests/test_job_runner_timeout.py | 21 +------------------ 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/packages/syft-job/src/syft_job/job_runner.py b/packages/syft-job/src/syft_job/job_runner.py index 0a281f78b23..f4b3c6b03a6 100644 --- a/packages/syft-job/src/syft_job/job_runner.py +++ b/packages/syft-job/src/syft_job/job_runner.py @@ -75,6 +75,7 @@ def _format_timeout(seconds: int) -> str: parts.append(f"{secs} second{'s' if secs != 1 else ''}") return " ".join(parts) + # A job runs from a fresh copy of its approved submission in the system temp # folder, outside the SyftBox: sync never reads it, and no submitter writes it. RUN_DIR_PREFIX = "syft-job-run-" diff --git a/packages/syft-job/tests/test_job_runner_timeout.py b/packages/syft-job/tests/test_job_runner_timeout.py index 77db7ab5e3a..d3a38dbefeb 100644 --- a/packages/syft-job/tests/test_job_runner_timeout.py +++ b/packages/syft-job/tests/test_job_runner_timeout.py @@ -41,30 +41,11 @@ def test_job_timeout_rejects_invalid_environment_value( @pytest.mark.parametrize( "seconds,expected", [ - (0, "0 seconds"), (30, "30 seconds"), - (1, "1 second"), (60, "1 minute"), (90, "1 minute 30 seconds"), - (120, "2 minutes"), - (600, "10 minutes"), - (3600, "1 hour"), (3661, "1 hour 1 minute 1 second"), - (7384, "2 hours 3 minutes 4 seconds"), ], ) -def test_format_timeout_produces_human_readable_string( - seconds: int, expected: str -) -> None: - """Timeouts should be formatted as readable hours, minutes, and seconds.""" +def test_format_timeout(seconds: int, expected: str) -> None: assert _format_timeout(seconds) == expected - - -def test_format_timeout_never_reports_zero_minutes_for_sub_minute_timeout() -> None: - """Regression test for issue #9527: sub-minute timeouts reported as 0 minutes.""" - for seconds in range(1, 60): - result = _format_timeout(seconds) - assert "0 minutes" not in result, ( - f"_format_timeout({seconds}) returned {result!r}, " - "which incorrectly contains '0 minutes'" - )