Pipe the solver's stderr instead of inheriting it - #222
Merged
Conversation
A solver timeout terminates the solver process itself, not anything that process spawned. `tests/thrust-pcsat-wrapper` spawns `docker run`, so a timed-out pcsat test leaves that container client running with the stderr it inherited, which is the pipe `ui_test` reads the whole `thrust-rustc` invocation from. `thrust-rustc` exits, but the survivor holds a write end open, so the read never reaches EOF and `cargo test` stops making progress. Give the solver its own stderr pipe. A survivor then holds a descriptor that dies with `thrust-rustc` rather than one the test harness waits on. It also fills in the `stderr` of `CheckSatError::Error`, which was always empty while the solver wrote straight to the inherited descriptor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FrCiKotN28PrBDKAheSAYc
Thirty-four ui tests solve inside the container `tests/thrust-pcsat-wrapper` runs. CI pins that image by digest and pulls it before `cargo test`; a session that does neither has each of those tests fall back to the `:main` tag and pull the image itself, under the solver timeout meant for solving. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FrCiKotN28PrBDKAheSAYc
coord-e
force-pushed
the
claude/cargo-test-local-hang-ep5285
branch
from
August 15, 2026 01:13
6ee7222 to
f2345cb
Compare
coord-e
marked this pull request as ready for review
August 15, 2026 01:49
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a local test-harness hang by ensuring the external CHC solver’s stderr is captured via a dedicated pipe rather than inherited from the thrust-rustc process, preventing orphaned descendants (e.g., a timed-out docker run) from keeping the harness’ output pipe open indefinitely.
Changes:
- Pipe the solver’s stderr (
Stdio::piped()) so orphaned descendants can’t hold open the harness’ inherited stderr pipe and so stderr is available forCheckSatError::Error. - Document the need to pull the pinned
COAR_IMAGEdigest before runningcargo testlocally.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/chc/solver.rs | Pipes solver stderr so solver output collection is self-contained and avoids ui_test hangs caused by inherited pipes held open by orphaned subprocesses. |
| CLAUDE.md | Adds local testing guidance to pre-pull the pinned COAR image to avoid routine timeouts/orphans during image pulls. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
cargo testsometimes stops making progress locally while CI stays green. The hang is notin the solver but in the pipe the test harness reads from.
A solver timeout terminates the solver process itself, not anything that process spawned.
tests/thrust-pcsat-wrapperspawnsdocker run, andCommandConfig::runset only stdout,leaving stderr inherited — which, under
ui_test, is the pipeCommand::output()reads thewhole
thrust-rustcinvocation from. So a timed-out pcsat test leaves the container clientrunning with a write end of that pipe.
thrust-rustcexits, the survivor keeps thedescriptor open, the read never reaches EOF, and
ui_testwaits forever. Plain z3 tests areunaffected: there the timeout kills the only holder of the pipe.
CI does not hit this because it pins
COAR_IMAGEby digest and pulls it beforecargo test,so the pcsat tests finish inside their timeout and nothing is orphaned. A local run without
that falls back to the
:maintag and pulls the image under the solver timeout, which makesthe timeouts — and the orphans — routine.
Give the solver its own stderr pipe. A survivor then holds a descriptor that dies with
thrust-rustcrather than one the test harness waits on. It also fills in thestderrofCheckSatError::Error, which was always empty while the solver wrote straight to theinherited descriptor.
The second commit records the image pull in
CLAUDE.md, next to the Z3 anddockerdsetupalready documented there.
Verification
Both directions were checked against the real binary, with a stand-in solver that reproduces
the wrapper's shape (a bash script whose grandchild outlives a
SIGKILLto it) driven throughthe same
Command::output()semanticsui_testuses, at a 5s solver timeout:thrust-rustcexits after 5s, the caller is still blocked at 60sThe full suite passes with Z3 5.0.0 and the pinned COAR image: 316 ui tests and 2 doc tests,
plus
cargo fmt --checkandcargo clippy -- -D warnings.Not addressed
This stops the harness from hanging; the orphaned
docker runand its container stillsurvive the timeout and keep consuming CPU, which can cascade into further timeouts. Killing
the whole process group, or cleaning up the container from the wrapper via
--cidfileand atrap, would be a separate change.
Generated by Claude Code