From bc86d4751d4594ac79fec65ca63fee726a700cf2 Mon Sep 17 00:00:00 2001 From: Jean-Paul van Ravensberg <14926452+DevSecNinja@users.noreply.github.com> Date: Sat, 17 Oct 2026 22:57:42 +0200 Subject: [PATCH 1/2] fix(mlx): address cache recovery and Python compatibility reviews Validate Qwen snapshots and resume missing files at the cached revision. Require Python 3.11, repair negative Bats assertions, and document a process-group timeout runner for macOS and Linux. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99d7cebd-e0b2-454f-babb-30613dcb1016 --- docs/development-tools.md | 19 ++++- home/Taskfile.yml | 6 +- home/dot_config/mlx/chat.py | 39 ++++++++- tests/bash/global-mlx-chat.bats | 44 ++++++++-- tests/python/test_mlx_chat.py | 140 ++++++++++++++++++++++++++++++-- tests/python/test_timeout.py | 79 ++++++++++++++++++ tests/with-timeout.py | 36 ++++++++ 7 files changed, 343 insertions(+), 20 deletions(-) create mode 100644 tests/python/test_timeout.py create mode 100644 tests/with-timeout.py diff --git a/docs/development-tools.md b/docs/development-tools.md index 8f38a529..6f150c8d 100644 --- a/docs/development-tools.md +++ b/docs/development-tools.md @@ -70,7 +70,8 @@ symlink redirection. A `status` check then verifies the venv and imports the required MLX APIs without loading weights. A working installation skips provisioning entirely: no Python replacement, pip upgrade, or package upgrade. -If the venv is missing, Task creates it with native Python 3.10+ and `venv`. +If the venv is missing, Task creates it with native Python 3.11+ and `venv` +(the minimum required by `mlx-lm` 0.32.2). If suitable Python is missing, it uses the same Homebrew `python@3` formula as the repository's macOS package setup. Missing pip is bootstrapped with `ensurepip`; missing requirements are installed with isolated @@ -103,6 +104,14 @@ which supplies an empty closed `` block. The launcher uses temperature 0.7, top-p 0.8, top-k 20, at most 512 reply tokens, and 512-token prefill chunks. It does not use the unsupported `mlx_lm.chat` flags or a `/nothink` soft switch. +A cached snapshot is reused without network access when its configuration, +tokenizer, chat template, weight index and every indexed shard are present. +Missing files trigger one resumable download into the same external cache, +at the cached revision when its directory was returned by the Hub. Already +cached files are preserved. If the snapshot remains incomplete (for +example while offline), the launcher reports an error rather than loading it. +Malformed metadata and model-loading errors are not treated as cache misses. + Type `q` to quit, `r` to reset, or `h` for help. Ctrl+C (including during generation) and EOF exit cleanly. Each reply reports token speed and peak MLX memory; conversation state stays in memory, with no saved transcripts. @@ -154,9 +163,15 @@ isolated Chezmoi destination. It covers installed/missing/broken environments and setup ordering without real provisioning, downloads, or inference: ```sh -timeout 60 ./tests/bash/run-tests.sh --test global-mlx-chat.bats +python3 tests/with-timeout.py 60 ./tests/bash/run-tests.sh --test global-mlx-chat.bats ``` +The Python standard-library wrapper works on macOS and Linux without GNU +`timeout`. It preserves the command's exit status (128 + signal for a signal +exit), kills the entire test process group on timeout, and returns 124. +Ctrl+C also cleans up the process group and returns 130. Use 300 instead of +60 for the full `--ci` suite. + ## Mise (Tool Version Manager) Mise manages tool versions defined in diff --git a/home/Taskfile.yml b/home/Taskfile.yml index 6ed39800..a632ad22 100644 --- a/home/Taskfile.yml +++ b/home/Taskfile.yml @@ -4,7 +4,7 @@ vars: MLX_VENV: '{{.ROOT_DIR}}/mlx-qwen/.venv' MLX_VOLUME: /Volumes/External Disk Data MLX_CACHE_DIR: '{{.MLX_VOLUME}}/AI/huggingface' - MLX_PYTHON_CHECK: 'import platform, sys, venv; assert sys.version_info >= (3, 10) and platform.machine() == "arm64"' + MLX_PYTHON_CHECK: 'import platform, sys, venv; assert sys.version_info >= (3, 11) and platform.machine() == "arm64"' MLX_VENV_CHECK: '{{.MLX_PYTHON_CHECK}}; assert sys.prefix != sys.base_prefix' MLX_CHECK_ENV: >- "{{.MLX_VENV}}/bin/python" "{{.ROOT_DIR}}/.config/mlx/chat.py" @@ -71,7 +71,7 @@ tasks: - sh: >- (test ! -e "{{.MLX_VENV}}" && test ! -L "{{.MLX_VENV}}") || "{{.MLX_VENV}}/bin/python" -c '{{.MLX_VENV_CHECK}}' - msg: 'Existing {{.MLX_VENV}} is not a usable Python 3.10+ arm64 venv. Repair it or move it aside; it will not be deleted automatically.' + msg: 'Existing {{.MLX_VENV}} is not a usable Python 3.11+ arm64 venv. Repair it or move it aside; it will not be deleted automatically.' status: - '"{{.MLX_VENV}}/bin/python" -c ''{{.MLX_VENV_CHECK}}''' cmds: @@ -88,7 +88,7 @@ tasks: internal: true preconditions: - sh: python3 -c '{{.MLX_PYTHON_CHECK}}' 2>/dev/null || command -v brew - msg: 'A native Python 3.10+ or Homebrew is required. Install Homebrew using the existing dotfiles setup; this task cannot safely bootstrap Homebrew.' + msg: 'A native Python 3.11+ or Homebrew is required. Install Homebrew using the existing dotfiles setup; this task cannot safely bootstrap Homebrew.' status: - | python3 -c '{{.MLX_PYTHON_CHECK}}' 2>/dev/null || diff --git a/home/dot_config/mlx/chat.py b/home/dot_config/mlx/chat.py index e9b3add5..3ff3e57b 100644 --- a/home/dot_config/mlx/chat.py +++ b/home/dot_config/mlx/chat.py @@ -3,6 +3,7 @@ import argparse from contextlib import closing from importlib.metadata import version, PackageNotFoundError +import json import os from pathlib import Path import platform @@ -10,6 +11,10 @@ MODEL = "mlx-community/Qwen3.5-9B-4bit" +REQUIRED_FILES = ( + "config.json", "tokenizer.json", "tokenizer_config.json", + "chat_template.jinja", "model.safetensors.index.json", +) CONTEXT_TOKENS = 8192 REPLY_TOKENS = 512 HELP = "q: quit | r: reset conversation | h: help" @@ -47,8 +52,8 @@ def configure_cache(cache_dir, volume): def check_environment(): - if sys.version_info < (3, 10) or sys.prefix == sys.base_prefix: - raise ValueError("A Python 3.10+ virtual environment is required.") + if sys.version_info < (3, 11) or sys.prefix == sys.base_prefix: + raise ValueError("A Python 3.11+ virtual environment is required.") import mlx.core from mlx_lm import load, stream_generate from mlx_lm.models.cache import make_prompt_cache @@ -59,6 +64,19 @@ def check_environment(): raise ValueError("mlx-lm 0.32.2 or newer is required for this launcher.") +def snapshot_is_complete(snapshot): + """Check Qwen3.5's required files, not the integrity of their contents.""" + root = Path(snapshot) + # Require the index even if a shard exists: a partial download may omit it. + if not all((root / name).is_file() for name in REQUIRED_FILES): + return False + with (root / "model.safetensors.index.json").open() as index: + shards = set(json.load(index)["weight_map"].values()) + if not shards: + raise ValueError("Cached model weight index contains no shards.") + return all((root / shard).is_file() for shard in shards) + + def chat(model, tokenizer): import mlx.core as mx from mlx_lm import stream_generate @@ -182,15 +200,28 @@ def main(): from mlx_lm import load except ImportError as error: parser.exit(1, f"Cannot import mlx-lm from this Python environment: {error}\n") + revision = None + allow_patterns = [*REQUIRED_FILES, "model*.safetensors"] try: snapshot = snapshot_download( - MODEL, cache_dir=str(root / "hub"), local_files_only=True + MODEL, cache_dir=str(root / "hub"), local_files_only=True, + allow_patterns=allow_patterns, ) + if not snapshot_is_complete(snapshot): + # Hub cache snapshot directories are named by their commit hash. + revision = Path(snapshot).name + raise LocalEntryNotFoundError("Cached Qwen3.5 snapshot is incomplete.") except LocalEntryNotFoundError: print(f"Downloading {MODEL} to {root / 'hub'} (external SSD)...", flush=True) snapshot = snapshot_download( - MODEL, cache_dir=str(root / "hub"), local_files_only=False + MODEL, cache_dir=str(root / "hub"), local_files_only=False, + allow_patterns=allow_patterns, revision=revision, ) + if not snapshot_is_complete(snapshot): + raise FileNotFoundError( + "Qwen3.5 snapshot is still incomplete after download. " + "Check connectivity and retry; the external cache has been preserved." + ) print(f"Loading {MODEL} from the external cache...", flush=True) model, tokenizer = load( diff --git a/tests/bash/global-mlx-chat.bats b/tests/bash/global-mlx-chat.bats index 439ecf42..960af75d 100644 --- a/tests/bash/global-mlx-chat.bats +++ b/tests/bash/global-mlx-chat.bats @@ -8,6 +8,8 @@ setup() { MLX_TEST_VOLUME="$(cd "$BATS_TEST_TMPDIR/External Disk Data" && pwd -P)" CACHE_DIR="$MLX_TEST_VOLUME/AI/huggingface" export MOCK_OS=Darwin MOCK_ARCH=arm64 MOCK_MOUNTED=1 MOCK_BASE_PYTHON=1 + export MOCK_BASE_PYTHON_VERSION=3.14 MLX_TEST_REAL_PYTHON + MLX_TEST_REAL_PYTHON="$(tool_binary python3)" export PYTHON_CALLS="$BATS_TEST_TMPDIR/python-calls" export MLX_TEST_ACTIONS="$BATS_TEST_TMPDIR/actions" export MLX_TEST_STATE="$BATS_TEST_TMPDIR/state" @@ -72,6 +74,7 @@ case "$1" in -c) case "$0" in "$MLX_TEST_BREW"/*) exit 0 ;; esac [ "$MOCK_BASE_PYTHON" -eq 1 ] || exit 1 + "$MLX_TEST_REAL_PYTHON" -c 'import platform,sys; sys.version_info=tuple(map(int,sys.argv[1].split("."))); platform.machine=lambda: "arm64"; exec(sys.argv[2])' "$MOCK_BASE_PYTHON_VERSION" "$2" ;; -m) [ "$2" = venv ] || exit 1 @@ -119,7 +122,7 @@ global_task() { @test "global-mlx-chat: Python behavior is validated without MLX or model weights" { command -v python3 >/dev/null || skip "Python 3 not installed" - run python3 -B -m unittest discover -s "$REPO_ROOT/tests/python" -p test_mlx_chat.py + run python3 -B -m unittest discover -s "$REPO_ROOT/tests/python" -p 'test_*.py' [ "$status" -eq 0 ] } @@ -266,7 +269,8 @@ EOF grep -F -- "--isolated --require-virtualenv" "$MLX_TEST_ACTIONS" grep -F -- "--cache-dir $CACHE_DIR/pip install mlx-lm>=0.32.2" "$MLX_TEST_ACTIONS" grep -F -- "tmp: $CACHE_DIR/tmp" "$MLX_TEST_ACTIONS" - ! grep -F -- "--upgrade" "$MLX_TEST_ACTIONS" + run grep -F -- "--upgrade" "$MLX_TEST_ACTIONS" + [ "$status" -eq 1 ] : >"$MLX_TEST_ACTIONS" run global_task llm:chat @@ -283,8 +287,10 @@ chat" ] run global_task llm:chat [ "$status" -eq 0 ] [[ "$(cat "$MLX_TEST_ACTIONS")" == *check-pip*ensurepip*pip:*check-env*chat ]] - ! grep -F -- "create-venv" "$MLX_TEST_ACTIONS" - ! grep -F -- "install-python" "$MLX_TEST_ACTIONS" + run grep -F -- "create-venv" "$MLX_TEST_ACTIONS" + [ "$status" -eq 1 ] + run grep -F -- "install-python" "$MLX_TEST_ACTIONS" + [ "$status" -eq 1 ] } @test "global-mlx-chat: unusable packages after pip fail the post-install check" { @@ -318,6 +324,30 @@ chat" ] [[ "$(cat "$MLX_TEST_ACTIONS")" == mount*install-python*create-venv*pip:*chat ]] } +@test "global-mlx-chat: Python 3.10 falls back to Homebrew before creating a venv" { + command -v task >/dev/null || skip "Task not installed" + rm "$TEST_HOME/mlx-qwen/.venv/bin/python" + rmdir "$TEST_HOME/mlx-qwen/.venv/bin" "$TEST_HOME/mlx-qwen/.venv" + printf 'missing\n' >"$MLX_TEST_STATE" + export MOCK_BASE_PYTHON_VERSION=3.10 + run global_task llm:chat + [ "$status" -eq 0 ] + [[ "$(cat "$MLX_TEST_ACTIONS")" == mount*install-python*create-venv*pip:*chat ]] +} + +@test "global-mlx-chat: Python 3.11 is accepted without installing Python" { + command -v task >/dev/null || skip "Task not installed" + rm "$TEST_HOME/mlx-qwen/.venv/bin/python" + rmdir "$TEST_HOME/mlx-qwen/.venv/bin" "$TEST_HOME/mlx-qwen/.venv" + printf 'missing\n' >"$MLX_TEST_STATE" + export MOCK_BASE_PYTHON_VERSION=3.11 + run global_task llm:chat + [ "$status" -eq 0 ] + grep -F -- "create-venv" "$MLX_TEST_ACTIONS" + run grep -F -- "install-python" "$MLX_TEST_ACTIONS" + [ "$status" -eq 1 ] +} + @test "global-mlx-chat: absent Python and Homebrew are reported without unsafe bootstrap" { command -v task >/dev/null || skip "Task not installed" rm "$TEST_HOME/mlx-qwen/.venv/bin/python" "$BATS_TEST_TMPDIR/bin/brew" @@ -327,7 +357,8 @@ chat" ] run global_task llm:chat [ "$status" -ne 0 ] [[ "$output" == *"cannot safely bootstrap Homebrew"* ]] - ! grep -F -- "install-python" "$MLX_TEST_ACTIONS" + run grep -F -- "install-python" "$MLX_TEST_ACTIONS" + [ "$status" -eq 1 ] [ ! -e "$PYTHON_CALLS" ] } @@ -342,7 +373,8 @@ chat" ] run global_task llm:chat [ "$status" -eq 0 ] grep -F -- "create-venv" "$MLX_TEST_ACTIONS" - ! grep -F -- "install-python" "$MLX_TEST_ACTIONS" + run grep -F -- "install-python" "$MLX_TEST_ACTIONS" + [ "$status" -eq 1 ] } @test "global-mlx-chat: cache outside the external volume is rejected before setup" { diff --git a/tests/python/test_mlx_chat.py b/tests/python/test_mlx_chat.py index 50d1270e..12388c60 100644 --- a/tests/python/test_mlx_chat.py +++ b/tests/python/test_mlx_chat.py @@ -3,6 +3,7 @@ from contextlib import redirect_stdout, redirect_stderr import importlib.util import io +import json import os from pathlib import Path import sys @@ -17,6 +18,10 @@ launcher = importlib.util.module_from_spec(SPEC) SPEC.loader.exec_module(launcher) CHECK_ENVIRONMENT = launcher.check_environment +ALLOW_PATTERNS = [ + "config.json", "tokenizer.json", "tokenizer_config.json", + "chat_template.jinja", "model.safetensors.index.json", "model*.safetensors", +] class LocalEntryNotFoundError(Exception): @@ -227,11 +232,29 @@ def run_main(self, root, system="Darwin", machine="arm64", check=False): ): launcher.main() + def cached_snapshot(self, root): + snapshot = root / "hub/models--mlx-community--Qwen3.5-9B-4bit/snapshots" / ("a" * 40) + snapshot.mkdir(parents=True) + for name in ( + "config.json", "tokenizer.json", "tokenizer_config.json", "chat_template.jinja", + "model-00001-of-00002.safetensors", "model-00002-of-00002.safetensors", + ): + (snapshot / name).write_text("{}") + (snapshot / "model.safetensors.index.json").write_text(json.dumps({ + "weight_map": { + "layer.0": "model-00001-of-00002.safetensors", + "layer.1": "model-00002-of-00002.safetensors", + } + })) + self.snapshot.return_value = str(snapshot) + return snapshot + def test_main_forces_external_cache_and_uses_existing_model_without_download(self): with tempfile.TemporaryDirectory(prefix="mlx cache ") as directory: root = Path(directory).resolve() (root / "hub").mkdir() (root / "xet").mkdir() + cached = self.cached_snapshot(root) with patch.dict(os.environ, { "HF_HOME": "/internal", "HF_HUB_CACHE": "/internal/hub", "HF_XET_CACHE": "/internal/xet", "HF_HUB_OFFLINE": "1", @@ -240,19 +263,20 @@ def test_main_forces_external_cache_and_uses_existing_model_without_download(sel def snapshot(*args, **kwargs): self.assertEqual(args, (launcher.MODEL,)) self.assertEqual(kwargs, { - "cache_dir": str(root / "hub"), "local_files_only": True + "cache_dir": str(root / "hub"), "local_files_only": True, + "allow_patterns": ALLOW_PATTERNS, }) self.assertEqual(os.environ["HF_HOME"], str(root)) self.assertEqual(os.environ["HF_HUB_CACHE"], str(root / "hub")) self.assertEqual(os.environ["HF_XET_CACHE"], str(root / "xet")) self.assertEqual(os.environ["HF_HUB_OFFLINE"], "0") self.assertEqual(os.environ["TRANSFORMERS_OFFLINE"], "0") - return str(root / "hub/snapshot") + return str(cached) self.snapshot.side_effect = snapshot self.run_main(root) self.snapshot.assert_called_once() self.load.assert_called_once_with( - str(root / "hub/snapshot"), + str(cached), tokenizer_config={"trust_remote_code": False}, trust_remote_code=False, ) @@ -277,17 +301,107 @@ def test_missing_model_downloads_only_to_external_cache(self): root = Path(directory).resolve() (root / "hub").mkdir() (root / "xet").mkdir() + cached = self.cached_snapshot(root) self.snapshot.side_effect = [ - LocalEntryNotFoundError("not cached"), str(root / "hub/snapshot") + LocalEntryNotFoundError("not cached"), str(cached) ] self.run_main(root) self.assertEqual(self.snapshot.call_count, 2) self.assertEqual(self.snapshot.call_args.kwargs, { - "cache_dir": str(root / "hub"), "local_files_only": False + "cache_dir": str(root / "hub"), "local_files_only": False, + "allow_patterns": ALLOW_PATTERNS, "revision": None, }) self.assertIn("Downloading", self.output.getvalue()) self.load.assert_called_once() + def test_partial_snapshots_resume_once_and_reuse_completed_files(self): + for missing in ( + "config.json", "tokenizer.json", "tokenizer_config.json", "chat_template.jinja", + "model.safetensors.index.json", "model-00002-of-00002.safetensors", + ): + with self.subTest(missing=missing), tempfile.TemporaryDirectory() as directory: + root = Path(directory).resolve() + (root / "xet").mkdir() + cached = self.cached_snapshot(root) + existing = cached / "model-00001-of-00002.safetensors" + before = existing.stat() + content = (cached / missing).read_bytes() + (cached / missing).unlink() + self.snapshot.reset_mock(side_effect=True) + self.load.reset_mock() + + def download(model, *, cache_dir, local_files_only, allow_patterns, revision=None): + self.assertEqual(model, launcher.MODEL) + self.assertEqual(cache_dir, str(root / "hub")) + self.assertEqual(allow_patterns, ALLOW_PATTERNS) + if not local_files_only: + self.assertEqual(revision, cached.name) + (cached / missing).write_bytes(content) + return str(cached) + + self.snapshot.side_effect = download + with patch.dict(os.environ): + self.run_main(root) + self.run_main(root) + self.assertEqual( + [call.kwargs["local_files_only"] for call in self.snapshot.call_args_list], + [True, False, True], + ) + self.assertEqual(self.load.call_count, 2) + self.assertEqual(existing.stat().st_ino, before.st_ino) + self.assertEqual(existing.stat().st_mtime_ns, before.st_mtime_ns) + + def test_empty_snapshot_and_broken_shard_symlink_are_incomplete(self): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + self.assertFalse(launcher.snapshot_is_complete(root)) + cached = self.cached_snapshot(root) + shard = cached / "model-00002-of-00002.safetensors" + shard.unlink() + shard.symlink_to("../../blobs/missing") + self.assertFalse(launcher.snapshot_is_complete(cached)) + self.assertTrue((cached / "model-00001-of-00002.safetensors").is_file()) + + def test_offline_partial_snapshot_fails_after_one_retry_without_loading(self): + with tempfile.TemporaryDirectory() as directory, patch.dict(os.environ): + root = Path(directory).resolve() + (root / "xet").mkdir() + cached = self.cached_snapshot(root) + (cached / "model-00002-of-00002.safetensors").unlink() + # Hub 1.5.0 can return the same partial folder even on the online call. + with self.assertRaisesRegex(FileNotFoundError, "still incomplete"): + self.run_main(root) + self.assertEqual(self.snapshot.call_count, 2) + self.load.assert_not_called() + self.assertTrue((cached / "model-00001-of-00002.safetensors").is_file()) + + def test_malformed_index_is_not_retried_as_a_cache_miss(self): + for content, error in ( + ("{", json.JSONDecodeError), ("{}", KeyError), + ('{"weight_map": {}}', ValueError), + ): + with self.subTest(content=content), tempfile.TemporaryDirectory() as directory: + root = Path(directory).resolve() + (root / "xet").mkdir() + cached = self.cached_snapshot(root) + (cached / "model.safetensors.index.json").write_text(content) + self.snapshot.reset_mock() + with patch.dict(os.environ), self.assertRaises(error): + self.run_main(root) + self.snapshot.assert_called_once() + self.load.assert_not_called() + + def test_cache_lookup_errors_are_not_retried(self): + with tempfile.TemporaryDirectory() as directory, patch.dict(os.environ): + root = Path(directory) + (root / "hub").mkdir() + (root / "xet").mkdir() + self.snapshot.side_effect = PermissionError("cache not readable") + with self.assertRaisesRegex(PermissionError, "cache not readable"): + self.run_main(root) + self.snapshot.assert_called_once() + self.load.assert_not_called() + def test_download_failure_is_not_hidden(self): with tempfile.TemporaryDirectory() as directory, patch.dict(os.environ): root = Path(directory) @@ -334,6 +448,19 @@ def test_environment_check_accepts_current_and_newer_without_installing(self): self.load.assert_not_called() self.snapshot.assert_not_called() + def test_environment_check_rejects_python_310_and_accepts_311(self): + with patch.object(sys, "prefix", "/mock/venv"): + with patch.object(sys, "version_info", (3, 10, 19)): + with self.assertRaisesRegex(ValueError, "Python 3.11"): + CHECK_ENVIRONMENT() + with ( + patch.object(sys, "version_info", (3, 11, 14)), + patch.object(launcher, "version", return_value="0.32.2"), + ): + CHECK_ENVIRONMENT() + self.load.assert_not_called() + self.snapshot.assert_not_called() + def test_environment_check_rejects_old_packages_and_non_venv_python(self): with ( patch.object(sys, "version_info", (3, 14, 8)), @@ -375,9 +502,12 @@ def test_model_loading_failure_propagates(self): root = Path(directory) (root / "hub").mkdir() (root / "xet").mkdir() + self.cached_snapshot(root) self.load.side_effect = RuntimeError("incomplete model weights") with self.assertRaisesRegex(RuntimeError, "incomplete model weights"): self.run_main(root) + self.snapshot.assert_called_once() + self.load.assert_called_once() if __name__ == "__main__": diff --git a/tests/python/test_timeout.py b/tests/python/test_timeout.py new file mode 100644 index 00000000..3d3a1d5c --- /dev/null +++ b/tests/python/test_timeout.py @@ -0,0 +1,79 @@ +"""Process-level regressions for the documented macOS/Linux timeout command.""" + +import os +from pathlib import Path +import signal +import subprocess +import sys +import tempfile +import time +import unittest + + +WRAPPER = Path(__file__).resolve().parents[1] / "with-timeout.py" + + +@unittest.skipUnless(os.name == "posix", "The validation wrapper targets macOS/Linux") +class TimeoutTests(unittest.TestCase): + def run_command(self, code, seconds="5"): + return subprocess.run( + [sys.executable, str(WRAPPER), seconds, sys.executable, "-c", code], + capture_output=True, text=True, timeout=10, + ) + + def test_preserves_success_failure_output_and_signal_exit(self): + for status in (0, 23): + with self.subTest(status=status): + result = self.run_command(f"print('visible output'); raise SystemExit({status})") + self.assertEqual(result.returncode, status) + self.assertEqual(result.stdout, "visible output\n") + result = self.run_command("import os,signal; os.kill(os.getpid(), signal.SIGTERM)") + self.assertEqual(result.returncode, 128 + signal.SIGTERM) + + def test_timeout_kills_child_and_grandchild(self): + # Inherited stdout stays open if only the immediate child is killed. + result = self.run_command( + "import subprocess,sys,time; " + "subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(30)']); " + "print('started', flush=True); time.sleep(30)", + seconds="1", + ) + self.assertEqual(result.returncode, 124) + self.assertIn("started", result.stdout) + self.assertIn("Validation timed out.", result.stdout) + + def test_interrupt_cleans_up_process_group(self): + with tempfile.TemporaryDirectory() as directory: + ready = Path(directory) / "ready" + code = ( + "import pathlib,subprocess,sys,time; " + "subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(30)']); " + f"pathlib.Path({str(ready)!r}).touch(); time.sleep(30)" + ) + with subprocess.Popen( + [sys.executable, str(WRAPPER), "5", sys.executable, "-c", code], + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + ) as wrapper: + deadline = time.monotonic() + 3 + while not ready.exists() and time.monotonic() < deadline: + time.sleep(0.01) + self.assertTrue(ready.exists(), "Child did not start") + wrapper.send_signal(signal.SIGINT) + output, error = wrapper.communicate(timeout=7) + self.assertEqual(wrapper.returncode, 130, error) + self.assertIn("Validation interrupted.", output) + + def test_invalid_timeout_and_missing_command_are_reported(self): + for seconds in ("0", "-1", "nan", "inf"): + with self.subTest(seconds=seconds): + result = self.run_command("raise SystemExit(99)", seconds) + self.assertEqual(result.returncode, 2) + self.assertIn("positive finite timeout", result.stderr) + result = subprocess.run( + [sys.executable, str(WRAPPER), "1"], capture_output=True, text=True, timeout=5, + ) + self.assertEqual(result.returncode, 2) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/with-timeout.py b/tests/with-timeout.py new file mode 100644 index 00000000..8273e92b --- /dev/null +++ b/tests/with-timeout.py @@ -0,0 +1,36 @@ +"""Run a validation command with a hard process-group timeout on macOS/Linux.""" + +import argparse +import math +import os +import signal +import subprocess + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("seconds", type=float) + parser.add_argument("command", nargs=argparse.REMAINDER) + args = parser.parse_args() + if not math.isfinite(args.seconds) or args.seconds <= 0 or not args.command: + parser.error("Provide a positive finite timeout and a command.") + + with subprocess.Popen(args.command, start_new_session=True) as process: + try: + code = process.wait(timeout=args.seconds) + except (subprocess.TimeoutExpired, KeyboardInterrupt) as error: + code = 124 if isinstance(error, subprocess.TimeoutExpired) else 130 + print( + "Validation timed out." if code == 124 else "Validation interrupted.", + flush=True, + ) + try: + os.killpg(process.pid, signal.SIGKILL) + except ProcessLookupError: + pass # The entire process group already exited. + process.wait() + return code if code >= 0 else 128 - code + + +if __name__ == "__main__": + raise SystemExit(main()) From d06fd668e10d0f65554dc94c3392b6cb4babbb70 Mon Sep 17 00:00:00 2001 From: Jean-Paul van Ravensberg <14926452+DevSecNinja@users.noreply.github.com> Date: Sat, 17 Oct 2026 23:15:06 +0200 Subject: [PATCH 2/2] fix(tests): preserve timeout cleanup with closed output pipes Terminate and reap validation children before best-effort unbuffered stderr diagnostics. Cover closed stdout and stderr during timeout and interruption, and skip the MLX fixture when Python is unavailable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99d7cebd-e0b2-454f-babb-30613dcb1016 --- docs/development-tools.md | 5 ++-- tests/bash/global-mlx-chat.bats | 19 ++++++++++++- tests/python/test_timeout.py | 49 ++++++++++++++++++++++++++++++--- tests/with-timeout.py | 10 ++++--- 4 files changed, 72 insertions(+), 11 deletions(-) diff --git a/docs/development-tools.md b/docs/development-tools.md index 6f150c8d..b0d659ef 100644 --- a/docs/development-tools.md +++ b/docs/development-tools.md @@ -169,8 +169,9 @@ python3 tests/with-timeout.py 60 ./tests/bash/run-tests.sh --test global-mlx-cha The Python standard-library wrapper works on macOS and Linux without GNU `timeout`. It preserves the command's exit status (128 + signal for a signal exit), kills the entire test process group on timeout, and returns 124. -Ctrl+C also cleans up the process group and returns 130. Use 300 instead of -60 for the full `--ci` suite. +Ctrl+C also cleans up the process group and returns 130. Diagnostics go to +stderr after cleanup; closed output pipes do not change either exit status. +Use 300 instead of 60 for the full `--ci` suite. ## Mise (Tool Version Manager) diff --git a/tests/bash/global-mlx-chat.bats b/tests/bash/global-mlx-chat.bats index 960af75d..8aa3a9ef 100644 --- a/tests/bash/global-mlx-chat.bats +++ b/tests/bash/global-mlx-chat.bats @@ -9,7 +9,7 @@ setup() { CACHE_DIR="$MLX_TEST_VOLUME/AI/huggingface" export MOCK_OS=Darwin MOCK_ARCH=arm64 MOCK_MOUNTED=1 MOCK_BASE_PYTHON=1 export MOCK_BASE_PYTHON_VERSION=3.14 MLX_TEST_REAL_PYTHON - MLX_TEST_REAL_PYTHON="$(tool_binary python3)" + MLX_TEST_REAL_PYTHON="$(tool_binary python3)" || skip "Python 3 not installed" export PYTHON_CALLS="$BATS_TEST_TMPDIR/python-calls" export MLX_TEST_ACTIONS="$BATS_TEST_TMPDIR/actions" export MLX_TEST_STATE="$BATS_TEST_TMPDIR/state" @@ -126,6 +126,23 @@ global_task() { [ "$status" -eq 0 ] } +@test "global-mlx-chat: unavailable Python skips instead of failing setup" { + local bats_bin tool tool_path bin + bats_bin="$(tool_binary bats)" + bin="$BATS_TEST_TMPDIR/without-python" + mkdir -p "$bin" + for tool in bash basename cat chmod cp cut date dirname env find grep head ln mkdir mktemp mv \ + nl paste readlink realpath rm rmdir sed sleep sort tail touch tr uname uniq wc; do + tool_path="$(command -v "$tool")" || continue + ln -s "$tool_path" "$bin/$tool" + done + [ ! -e "$bin/python3" ] + run env PATH="$bin" "$bats_bin" --tap \ + --filter "^global-mlx-chat: Python behavior is validated" "$BATS_TEST_FILENAME" + [ "$status" -eq 0 ] + [[ "$output" == *"# skip Python 3 not installed"* ]] +} + @test "global-mlx-chat: global task works outside the repository with spaces in paths" { command -v task >/dev/null || skip "Task not installed" run global_task llm:chat diff --git a/tests/python/test_timeout.py b/tests/python/test_timeout.py index 3d3a1d5c..a8a532f4 100644 --- a/tests/python/test_timeout.py +++ b/tests/python/test_timeout.py @@ -39,8 +39,8 @@ def test_timeout_kills_child_and_grandchild(self): seconds="1", ) self.assertEqual(result.returncode, 124) - self.assertIn("started", result.stdout) - self.assertIn("Validation timed out.", result.stdout) + self.assertEqual(result.stdout, "started\n") + self.assertIn("Validation timed out.", result.stderr) def test_interrupt_cleans_up_process_group(self): with tempfile.TemporaryDirectory() as directory: @@ -59,9 +59,50 @@ def test_interrupt_cleans_up_process_group(self): time.sleep(0.01) self.assertTrue(ready.exists(), "Child did not start") wrapper.send_signal(signal.SIGINT) - output, error = wrapper.communicate(timeout=7) + _, error = wrapper.communicate(timeout=7) self.assertEqual(wrapper.returncode, 130, error) - self.assertIn("Validation interrupted.", output) + self.assertIn("Validation interrupted.", error) + + def test_closed_output_pipes_do_not_bypass_cleanup_or_change_exit_status(self): + for interrupted in (False, True): + for stream in ("stdout", "stderr"): + with ( + self.subTest(interrupted=interrupted, stream=stream), + tempfile.TemporaryDirectory() as directory, + ): + ready = Path(directory) / "ready" + code = ( + "import os,pathlib,subprocess,sys,time; " + "subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(30)']); " + f"ready=pathlib.Path({str(ready)!r}); " + "ready.with_suffix('.tmp').write_text(str(os.getpid())); " + "ready.with_suffix('.tmp').rename(ready); time.sleep(30)" + ) + with subprocess.Popen( + [sys.executable, str(WRAPPER), "1", sys.executable, "-c", code], + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + ) as wrapper: + try: + deadline = time.monotonic() + 3 + while not ready.exists() and time.monotonic() < deadline: + time.sleep(0.01) + self.assertTrue(ready.exists(), "Child did not start") + getattr(wrapper, stream).close() + setattr(wrapper, stream, None) + if interrupted: + wrapper.send_signal(signal.SIGINT) + # The other pipe stays open if any descendant survives. + wrapper.communicate(timeout=3) + self.assertEqual(wrapper.returncode, 130 if interrupted else 124) + finally: + if ready.exists(): + try: + os.killpg(int(ready.read_text()), signal.SIGKILL) + except ProcessLookupError: + pass + if wrapper.poll() is None: + wrapper.kill() + wrapper.communicate(timeout=3) def test_invalid_timeout_and_missing_command_are_reported(self): for seconds in ("0", "-1", "nan", "inf"): diff --git a/tests/with-timeout.py b/tests/with-timeout.py index 8273e92b..4eab7ef3 100644 --- a/tests/with-timeout.py +++ b/tests/with-timeout.py @@ -20,15 +20,17 @@ def main(): code = process.wait(timeout=args.seconds) except (subprocess.TimeoutExpired, KeyboardInterrupt) as error: code = 124 if isinstance(error, subprocess.TimeoutExpired) else 130 - print( - "Validation timed out." if code == 124 else "Validation interrupted.", - flush=True, - ) try: os.killpg(process.pid, signal.SIGKILL) except ProcessLookupError: pass # The entire process group already exited. process.wait() + try: + os.write( + 2, b"Validation timed out.\n" if code == 124 else b"Validation interrupted.\n" + ) + except OSError: + pass # A closed diagnostic pipe must not replace the exit status. return code if code >= 0 else 128 - code