Skip to content

Kill forked submission processes after execution - #451

Closed
Sakshamm-Goyal wants to merge 2 commits into
Kattis:masterfrom
Sakshamm-Goyal:fix/kill-forked-tle-children
Closed

Kill forked submission processes after execution#451
Sakshamm-Goyal wants to merge 2 commits into
Kattis:masterfrom
Sakshamm-Goyal:fix/kill-forked-tle-children

Conversation

@Sakshamm-Goyal

Copy link
Copy Markdown

Summary

  • run each submission in its own session/process group
  • kill any descendants left behind after the direct submission process exits
  • add a regression that forks a sleeping child, simulates a CPU-limit signal, and verifies the child does not survive

Closes #450

Validation

  • fail-first regression: the forked child remained alive before the process-group cleanup
  • .venv/bin/python -m pytest -q tests/test_run_program.py tests/test_run_limit.py — 2 passed
  • .venv/bin/python -m pytest -q --ignore=tests/test_latex.py --ignore=tests/test_markdown.py --ignore=tests/test_xss.py — 67 passed
  • .venv/bin/ruff check .
  • .venv/bin/ruff format --check .
  • .venv/bin/mypy --non-interactive --config-file mypy.ini -p problemtools

The full local pytest run additionally needs lualatex; without it, the first PDF-render test fails and leaves the process in a removed temporary working directory, which cascades into 10 unrelated render-test failures.

Comment thread problemtools/run/program.py
Comment thread problemtools/run/program.py Outdated
Comment thread tests/test_run_program.py Outdated
Comment thread tests/test_run_program.py Outdated
Comment thread tests/test_run_program.py
Comment thread tests/test_run_program.py
@gkreitz

gkreitz commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR. This looks quite heavily like a bot, or at least pure LLM code. So I'm guessing PR comments won't get responses, and I'll close this somewhat swiftly if that seems to be the case.

Simon's comments look relevant. Also:

  • What other approaches did you consider, and why did you pick this one?
  • How does this PR affect pressing ctrl-c to abort a run (please test both with and without -j)?

@Sakshamm-Goyal

Sakshamm-Goyal commented Aug 8, 2026

Copy link
Copy Markdown
Author

The updated cleanup now:

  • uses os.setpgrp() instead of setsid(), so the submission gets a dedicated process group without detaching from the parent session/terminal;
  • observes the group leader with wait4(..., WNOWAIT), kills the whole process group, and then reaps the leader (with the macOS wait4 behavior handled explicitly);
  • applies the same process-group cleanup when the verifier is interrupted by Ctrl-C;
  • treats zombies as still present in the regression check, and writes the child PID through a temporary file plus os.replace() so the test cannot read a partial PID.

I considered killing only the direct PID (leaves forked descendants behind), enumerating descendants manually (racy), and setsid() (heavier and changes session/terminal semantics). A process group gives us the kernel-managed cleanup boundary while keeping the parent session intact.

I also tested Ctrl-C against a real verifyproblem run with both -j 1 and -j 2; in both cases the verifier exited with status 1 and neither the submission nor its forked child remained. The regression keeps the CPU limit real and uses the SIGXCPU path; it only bypasses the unrelated finite-stack-limit setting that macOS rejects.

Validation on the branch:

  • pytest -q tests/test_run_program.py tests/test_run_limit.py — 2 passed
  • pytest -q --ignore=tests/test_latex.py --ignore=tests/test_markdown.py --ignore=tests/test_xss.py — 67 passed
  • Ruff, formatting, compile, and git diff --check all pass

@gkreitz

gkreitz commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Code style is poor, you resolved multiple comments without actually addressing them. Not wasting time on this bot PR, closing.

@gkreitz gkreitz closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If a TLE submission forks, its children don't get killed

3 participants