Bound the prover subprocesses, and kill the tree when one wedges - #191
Open
shellygr wants to merge 1 commit into
Open
Bound the prover subprocesses, and kill the tree when one wedges#191shellygr wants to merge 1 commit into
shellygr wants to merge 1 commit into
Conversation
A run died quietly last week: the container sat for three days having used one second of CPU. `console-codegen` was in epoll_wait on its subprocess, certoraRunWrapper was blocked reading a pipe, and the Typechecker JVM behind it had all eighteen threads parked -- GC and VM threads included. Its last log line was `writing .certora_verify.json`, and every file in the build directory carried the same timestamp. Nothing could end that. All three `create_subprocess_exec` calls in this module awaited their child with no bound, so a local phase that stops responding without exiting takes the whole run with it. The `--global_timeout` in the command line is the cloud prover's budget, passed through to certoraRun; it bounds nothing locally, and this process never got as far as submitting a job. Children now run under a context manager that kills them on expiry, using the timeout the result poller already uses for the run path and a shorter fixed bound for the two build-and-typecheck helpers. A timeout on the run path comes back as an error string rather than an exception, so the agent reads it as a tool result and can retry instead of the pipeline stalling. The kill has to reach the grandchild. certoraRun is a python front end that shells out to a JVM, so signalling the direct child leaves that JVM holding the pipe its parent was read-waiting on -- which hangs the caller just as thoroughly as the original bug. Children are spawned as process-group leaders and killed by group. The tests bound themselves, because removing the group kill makes this hang rather than fail, and a test guarding a hang must not hang CI. Verified in both directions: four pass against the fix, and with the group kill removed two fail in thirty seconds instead of hanging. `fake_exec` in the rules-striping tests grows a `**kwargs` and an assertion on the new argument, so the double keeps matching the real call signature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happened
A codegen run stopped making progress and sat in that state for three days, having consumed one second of CPU. The process tree:
console-codegenepoll_wait— awaiting the subprocesscertoraRunWrapper.pypipe_read— reading the JVM's stderrjava Typechecker.jarThe JVM's last log line was
writing .certora_verify.json, and every file in the build directory shared one timestamp. Its stdout went to/dev/null, so this was not a pipe-buffer deadlock — the JVM itself had stopped.Nothing could end it. All three
create_subprocess_execcalls incomposer/prover/core.pyawaited their child with no bound, so a local phase that stops responding without exiting takes the run with it. The--global_timeout 7200visible in the command line is the cloud prover's budget, passed through tocertoraRun; it bounds nothing locally, and this process never reached the point of submitting a job.The change
A
_bounded_subprocesscontext manager wraps all three sites:prover_opts.global_timeout + 5 * 60— the same bound the result poller already uses at step 7On the run path a timeout returns an error string rather than raising, so the agent reads it as a tool result and can retry, instead of the pipeline stalling. The message says explicitly that this is infrastructure and not a problem with the spec — otherwise the author burns turns rewriting CVL that was fine.
The part that is easy to get wrong
Killing the direct child is not enough.
certoraRunis a python front end that shells out to a JVM. Signal only the child and the JVM survives, still holding the pipe its parent was read-waiting on — so the caller hangs exactly as it did before. Children are therefore spawned as process-group leaders (process_group=0) and killed by group.This is not theoretical: with the group kill removed, the helper hangs rather than failing, reproducing the original bug in a unit test.
Tests
tests/test_prover_subprocess_timeout.py— a hung child raises promptly, a grandchild standing in for the JVM is killed too, a healthy child is left alone, and the child genuinely leads its own process group.Every test bounds itself with
asyncio.timeout, because a regression here hangs instead of failing and a test guarding against a hang must not hang CI. Verified both ways:Full suite: 1045 passed, 49 skipped (
test_rag_db.pydeselected — it needs a Docker socket to spin up its own Postgres, unavailable when running inside the container).fake_execintests/test_rules_striping.pygains**kwargsplus an assertion on the new argument, so the double keeps matching the real call signature rather than silently drifting from it.Not in scope
The bound on the run path is deliberately generous — it is a backstop against a wedged process, not a budget, and a healthy cloud submit finishes in minutes. A tighter bound specifically for the cloud submit phase (which does not wait on the remote job at all) would catch this class of failure much sooner, but wants its own change.