adversarial: keep executor close bounded when a child holds stdout

[Jay (jay23606)]

Fixes #503 (executor close blocks on inherited stdout).

This replaces #532, which was closed. This branch is a single commit on current dev, with no merge commits.

Summary

• ExecutorClient.close() closed the buffered stdout stream after the wrapper exited. If a child it started still held stdout, the protocol reader was blocked in readline and held that stream's lock, so close() waited until the child exited (about 20 seconds in the reproduction).
• close() now joins the reader with its existing timeout first. If the reader is still blocked, stdout is not closed, and close() raises executor output is still held open by a surviving process. Otherwise stdout closes as before. The reader is a daemon thread and ends when the surviving process exits.
• Known limit: the surviving child is not terminated. Containing it (for example with a Windows job object) is a separate change.

Test

• ExecutorCloseTests.test_close_is_bounded_when_a_surviving_child_holds_executor_stdout runs a real wrapper that exits after starting a child that inherits stdout. Before the change, close() blocked about 20 seconds and returned without error. After the change it raises within the bound. The test kills the surviving child in a finally block.
• python -B -m unittest test_adversarial: 71 tests, OK (6 skipped, as before).
• ruff check and ruff format --check are clean for both files.
• Not run: the full nvx.py verify, which needs the OpenVMM submodule, and hardware-backed campaigns.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Deferred cleanup still permits unbounded response buffering and leaves stdout open after the reader exits.

2 open findings

• Stop retaining responses after client closure · New
• Close stdout when response reader exits · New

What changed in this PR

Addresses #503 by bounding executor cleanup when a surviving child holds stdout open.

Changes:

• Joins the protocol reader before closing stdout and reports a cleanup error if it remains blocked.
• Adds a real-process regression test for inherited stdout.

File	Description
scripts/​test_adversarial.py	Tests bounded closure with a surviving child.
scripts/​nvx_tools/​adversarial.py	Defers stdout closure until the reader finishes.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The regression test rejects a valid BrokenPipeError, causing scheduling-dependent failures on POSIX.

0 open findings

2 resolved since last review

• Stop retaining responses after client closure
• Close stdout when response reader exits

Previously missed (1)

In code that hasn't changed since last review

Accept BrokenPipeError when wrapper exits before request flushes

scripts/​test_adversarial.py:2638

On POSIX, the wrapper can exit before request() flushes its write. The shell redirects the background child's stdin to /dev/null, so the flush fails and leaves buffered data; _stdin.close() then raises BrokenPipeError. close() preserves that error rather than replacing it with ScriptError, even though cleanup finishes within two seconds. Accept BrokenPipeError here too, while retaining the elapsed-time assertion, so this valid scheduling order does not fail the regression test.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

The new regression test can fail on a valid BrokenPipeError cleanup path depending on process scheduling.

1 open issue

• Preserve ScriptError when closing stdin raises BrokenPipeError · New

_{Review effort: Balanced}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.