From 4dd4e06f1d5fd1d294cb82a84b803256760cbfff Mon Sep 17 00:00:00 2001 From: Liangsheng Yin Date: Wed, 25 Mar 2026 16:22:44 -0700 Subject: [PATCH] [CI] Fix resource leak when setUpClass fails (#21338) --- .claude/skills/write-sglang-test/SKILL.md | 16 ++++++----- python/sglang/test/test_utils.py | 27 +++++++++++++++++++ test/README.md | 1 + .../test_hicache_storage_file_backend.py | 6 +++-- 4 files changed, 42 insertions(+), 8 deletions(-) diff --git a/.claude/skills/write-sglang-test/SKILL.md b/.claude/skills/write-sglang-test/SKILL.md index 0dc8c40b3..c2d3e587b 100644 --- a/.claude/skills/write-sglang-test/SKILL.md +++ b/.claude/skills/write-sglang-test/SKILL.md @@ -9,10 +9,11 @@ description: Guide for writing SGLang CI/UT tests. Covers CustomTestCase, CI reg ## Core Rules -1. **Always use `CustomTestCase`** — never raw `unittest.TestCase` -2. **Place tests in `test/registered//`** — except JIT kernel tests and benchmarks, which live in `python/sglang/jit_kernel/tests/` and `python/sglang/jit_kernel/benchmark/` -3. **Reuse server fixtures** — inherit from `DefaultServerBase` or write `setUpClass`/`tearDownClass` with `popen_launch_server` -4. **Prefer mock over real server** — when testing logic that doesn't need a server / engine launch (middleware, request routing, config validation, argument parsing), use `unittest.mock.patch` / `MagicMock` and place tests in `test/registered/unit/`. Only launch a real server when the test genuinely needs inference results or server lifecycle behavior. +1. **Always use `CustomTestCase`** — never raw `unittest.TestCase`. It ensures `tearDownClass` runs even when `setUpClass` fails, preventing resource leaks in CI. +2. **`tearDownClass` must be defensive** — use `hasattr`/null checks before accessing resources (e.g. `cls.process`) that `setUpClass` may not have finished allocating. +3. **Place tests in `test/registered//`** — except JIT kernel tests and benchmarks, which live in `python/sglang/jit_kernel/tests/` and `python/sglang/jit_kernel/benchmark/` +4. **Reuse server fixtures** — inherit from `DefaultServerBase` or write `setUpClass`/`tearDownClass` with `popen_launch_server` +5. **Prefer mock over real server** — when testing logic that doesn't need a server / engine launch (middleware, request routing, config validation, argument parsing), use `unittest.mock.patch` / `MagicMock` and place tests in `test/registered/unit/`. Only launch a real server when the test genuinely needs inference results or server lifecycle behavior. JIT kernel exception: - If the task is adding or updating code under `python/sglang/jit_kernel/`, prefer the `add-jit-kernel` skill first. @@ -135,7 +136,8 @@ class TestMyFeature(CustomTestCase): @classmethod def tearDownClass(cls): - kill_process_tree(cls.process.pid) + if hasattr(cls, "process") and cls.process: + kill_process_tree(cls.process.pid) def test_basic_functionality(self): response = requests.post( @@ -183,7 +185,8 @@ class TestMyFeaturePerf(CustomTestCase): @classmethod def tearDownClass(cls): - kill_process_tree(cls.process.pid) + if hasattr(cls, "process") and cls.process: + kill_process_tree(cls.process.pid) def test_latency(self): start = time.perf_counter() @@ -311,5 +314,6 @@ Before submitting a test: - [ ] Backend-independent tests: `register_cuda_ci` only + smallest model - [ ] Logic that doesn't need a server / engine launch → unit test in `registered/unit/` (see Unit Tests section) - [ ] `setUpClass` launches server, `tearDownClass` kills it (if server-based) +- [ ] `tearDownClass` is defensive — uses `hasattr`/null checks before accessing resources that may not have been allocated - [ ] Has `if __name__ == "__main__": unittest.main()` - [ ] `est_time` is reasonable (measure locally) diff --git a/python/sglang/test/test_utils.py b/python/sglang/test/test_utils.py index 64822dd43..54efe47db 100644 --- a/python/sglang/test/test_utils.py +++ b/python/sglang/test/test_utils.py @@ -2083,6 +2083,33 @@ def _distributed_worker(rank, world_size, backend, port, func, result_queue, kwa class CustomTestCase(unittest.TestCase): + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + + # Wrap the effective setUpClass so that tearDownClass is called + # even when setUpClass fails. Python's unittest skips tearDownClass + # if setUpClass raises, which can leak resources (ports, processes). + setup = cls.setUpClass + if getattr(setup, "_safe_setup_wrapped", False): + return + + def safe_setUpClass(klass, _orig=setup): + try: + _orig.__func__(klass) + except Exception: + # Best-effort cleanup; suppress teardown errors so the + # original setUpClass exception propagates clearly. + try: + klass.tearDownClass() + except Exception: + pass + raise + + # Set sentinel on the raw function so that bound method attribute + # lookup (which delegates to __func__) can detect it in subclasses. + safe_setUpClass._safe_setup_wrapped = True + cls.setUpClass = classmethod(safe_setUpClass) + def _callTestMethod(self, method): max_retry = envs.SGLANG_TEST_MAX_RETRY.get() if max_retry is None: diff --git a/test/README.md b/test/README.md index 4b10a5eae..374643017 100644 --- a/test/README.md +++ b/test/README.md @@ -269,6 +269,7 @@ A scheduled job summarizes test coverage across all backends; [here is an exampl ## Tips for Writing Elegant Test Cases - Learn from existing examples in [test/registered](https://github.com/sgl-project/sglang/tree/main/test/registered). +- **Always use `CustomTestCase`** instead of raw `unittest.TestCase`, and make `tearDownClass` defensive (`hasattr` checks). - Reduce the test time by using smaller models and reusing the server for multiple test cases. Launching a server takes a lot of time, so please reuse a single server for many tests instead of launching many servers. - Use as few GPUs as possible. Use 1-GPU runners whenever possible. Do not run long tests with 8-gpu runners. - If the test cases take too long, consider adding them to nightly tests instead of per-commit tests. diff --git a/test/registered/hicache/test_hicache_storage_file_backend.py b/test/registered/hicache/test_hicache_storage_file_backend.py index 30d1239b7..afdce2cd4 100644 --- a/test/registered/hicache/test_hicache_storage_file_backend.py +++ b/test/registered/hicache/test_hicache_storage_file_backend.py @@ -63,11 +63,13 @@ class HiCacheStorageBaseMixin: @classmethod def tearDownClass(cls): """Clean up test environment""" - kill_process_tree(cls.process.pid) + if hasattr(cls, "process") and cls.process: + kill_process_tree(cls.process.pid) import shutil - shutil.rmtree(cls.temp_dir, ignore_errors=True) + if hasattr(cls, "temp_dir"): + shutil.rmtree(cls.temp_dir, ignore_errors=True) @classmethod def _get_model_name(cls):