Skip to content

Commit c68ed2b

Browse files
fix: revert SimpleQueue (get() lacks timeout param), keep spawn context fix
1 parent 96dae0a commit c68ed2b

2 files changed

Lines changed: 16 additions & 16 deletions

File tree

.github/workflows/ci.yml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,12 @@ jobs:
3737
run: mypy src/codefix_env/ --ignore-missing-imports
3838
continue-on-error: true # tighten later
3939

40-
- name: Run tests with coverage
40+
- name: Run tests
41+
run: |
42+
PYTHONPATH=src pytest tests/ -v --timeout=60 -p no:cacheprovider
43+
44+
- name: Run tests with coverage (py3.11 only)
45+
if: matrix.python-version == '3.11'
4146
run: |
4247
PYTHONPATH=src pytest tests/ -v --timeout=60 -p no:cacheprovider \
4348
--cov=src/codefix_env --cov-report=xml --cov-report=term

src/codefix_env/utils/sandbox.py

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -344,21 +344,15 @@ def run_code(code: str, test_code: str = "", timeout_s: float = 5.0) -> Executio
344344
if err := _validate_ast(full_code):
345345
return ExecutionResult(exception=err, passed=False)
346346

347-
# Run in child process for isolation.
348-
#
349-
# Explicitly use the "spawn" start method rather than the platform
350-
# default. On Linux, multiprocessing.Process() defaults to fork(),
351-
# which copies the ENTIRE parent memory image into every child --
352-
# including PyTorch, if anything in the same test/training session
353-
# already imported it (e.g. RewardMLP tests running earlier in the
354-
# same pytest process). Combined with coverage.py tracing every
355-
# forked child, running ~200 sandboxed executions in one CI session
356-
# compounded into an OOM kill on GitHub's 7GB runner, confirmed by
357-
# the kill point moving later (not disappearing) after the first
358-
# resource-cleanup fix -- the leak was real but this was the larger
359-
# cause. spawn starts each child as a fresh, minimal interpreter that
360-
# does not inherit torch or the parent's coverage trace state, at the
361-
# cost of slightly higher per-call startup time versus fork.
347+
# spawn (not fork, see comment history) for a fresh child interpreter
348+
# per execution, avoiding inherited torch/coverage state from the
349+
# parent. Reverted the earlier SimpleQueue attempt: SimpleQueue.get()
350+
# does not accept a timeout argument (unlike Queue.get()), which made
351+
# every single call raise TypeError and get swallowed by the except
352+
# below as a spurious "process exited without result" -- confirmed by
353+
# every test failing identically, including trivially fast ones that
354+
# have no plausible resource-pressure explanation. Queue's feeder
355+
# thread overhead is the accepted cost of keeping timeout() support.
362356
ctx = multiprocessing.get_context("spawn")
363357
q: multiprocessing.Queue = ctx.Queue()
364358
proc = ctx.Process(
@@ -392,6 +386,7 @@ def run_code(code: str, test_code: str = "", timeout_s: float = 5.0) -> Executio
392386
# even though every individual test passes — confirmed by the
393387
# process being killed only after all 202 tests had already
394388
# completed, during coverage report generation.
389+
# SimpleQueue has no .join_thread() (no feeder thread to join).
395390
q.close()
396391
q.join_thread()
397392
if proc.is_alive():

0 commit comments

Comments
 (0)