Repository navigation
Runtime: optional runtime-owned test execution - #29
tydinjarman wants to merge 2 commits into
Conversation
Add an opt-in hook that runs a verification command when a coding worker completes, with a timeout. Disabled by default via typed config (`verify_tests_on_complete`, `test_command_timeout`, default 120s). On timeout the subprocess is fully reaped: POSIX kills the whole process group (child starts with start_new_session=True) via SIGTERM then SIGKILL and always wait()s -- no zombies, no orphaned children; Windows uses terminate/kill + wait. Never raises for command failures; surfaces a structured VerificationOutcome (completed/timed_out/ launch_failed, returncode, counts, summary) and emits a TEST_RESULT event as evidence only, never as a gate.
JARosen
left a comment
There was a problem hiding this comment.
The verification subprocess currently starts after WORKER_COMPLETED has already entered the supervisor queue. That creates a lifecycle race: Foreman can assess the completion event, select FINISH, and close while verification is still running. Because the worker has already been removed from active_workers, shutdown does not wait for that task; cancellation of run_verification_command() also does not terminate and reap its subprocess.
Please make verification part of the worker terminal lifecycle—finishing before the terminal event is emitted, or represented as an explicitly tracked active task—and guarantee subprocess cleanup on cancellation. An end-to-end test should demonstrate that the factory cannot finish before the TEST_RESULT is recorded.
…ycle The race: WORKER_COMPLETED entered the supervisor queue before runtime verification ran, so the supervisor could assess the completion, select FINISH, and close while verification was still active - with the worker already removed from active_workers, shutdown never waited, and cancelling run_verification_command() left its subprocess unterminated. Fix: _run_worker() now runs _verify_tests_after_worker() while the worker is still tracked in active_workers, and the TEST_RESULT event is recorded before the terminal WORKER_COMPLETED event (which now carries a nested verification summary: status, returncode, passed/failed/errored/skipped, summary). Since the worker task remains in active_workers throughout, close()/_terminate_active() waits for in-flight verification instead of racing past it. run_verification_command() catches asyncio.CancelledError, terminates and reaps the subprocess (and its POSIX process group) via the existing _terminate_process_tree(), then re-propagates; _run_worker() captures verification-time cancellation so lifecycle bookkeeping and the terminal event still complete before re-raising. Tests: 182 passed (13/13 in test_runtime_verification.py), including two new ones: cancellation of run_verification_command() reaps the child (PID verified gone via /proc), and an end-to-end two-worker test proving TEST_RESULT precedes WORKER_COMPLETED precedes FINISH, and that shutdown waits for a still-in-flight verification on another worker (its TEST_RESULT lands after FACTORY_FINISHED, uncancelled).
tydinjarman
left a comment
There was a problem hiding this comment.
Addressed the lifecycle race (pushed as commit 80d4b2e).
You were right about the race: WORKER_COMPLETED entered the supervisor
queue before verification ran, so the supervisor could assess the
completion, select FINISH, and close while verification was still active —
with the worker already removed from active_workers, shutdown never
waited, and cancelling run_verification_command() left its subprocess
unterminated.
Verification is now part of the worker terminal lifecycle: _run_worker()
runs _verify_tests_after_worker() while the worker is still tracked in
active_workers, and the TEST_RESULT event is recorded before the
terminal WORKER_COMPLETED event (which now carries a nested verification
summary: status, returncode, passed/failed/errored/skipped, summary). Since
the worker task remains in active_workers throughout,
close()/_terminate_active() waits for in-flight verification instead of
racing past it. run_verification_command() catches
asyncio.CancelledError, terminates and reaps the subprocess (and its
POSIX process group) via the existing _terminate_process_tree(), then
re-propagates; _run_worker() captures verification-time cancellation so
lifecycle bookkeeping and the terminal event still complete before
re-raising. docs/runtime.md documents the terminal-lifecycle ordering and
cancellation cleanup.
Test evidence: full suite 182 passed (13/13 in
test_runtime_verification.py), including two new tests — cancellation of
run_verification_command() reaps the child (PID verified gone via
/proc), and an end-to-end two-worker test proving TEST_RESULT
precedes WORKER_COMPLETED precedes FINISH, and that shutdown waits for
a still-in-flight verification on another worker (its TEST_RESULT lands
after FACTORY_FINISHED, uncancelled). Ruff clean.
One note: test_integration.py::test_noisy_events_are_coalesced flakes
rarely (~2–5% of runs) in a way unrelated to this change — the diff is
provably inert on that path (verification-gated code never enabled by that
test). Left alone as out of scope.
|
Thank you for addressing the original lifecycle race and cancellation cleanup so thoroughly. The revised ordering is much safer. Before another implementation round, I think we should pause on two broader issues. First, the coding backend remains in Second, now that #28 has merged, this should reuse the shared pytest-summary parser rather than maintain a second parser in There is also a product-boundary question I should resolve before asking you to revise again: core runtime execution of a fixed Please hold off on another rework for the moment. I would rather settle that boundary clearly than send you through incremental patches. |
|
Following up on my request to pause: Foreman now supports centrally configured command evidence providers, selected by checks and executed before the completion assessment. That gives test execution a repository-agnostic home within the existing evidence architecture. I think this largely supersedes the runtime-specific hook proposed here. Do you see any capability from this PR that remains uncovered? If so, a focused follow-up through the evidence-provider path would be welcome. Otherwise, I suggest closing this as superseded. Thank you for the work on lifecycle ordering and subprocess cleanup. |
Supersedes part of #20, per your review: this is the focused "optional runtime-owned test execution" change, rebased on current
main, with no Hermes or policy changes and no helper scripts.What it does
src/foreman/runtime.py: when a coding worker completes, optionally run a verification command with a timeout. Disabled by default (verify_tests_on_complete: bool = False,FOREMAN_VERIFY_TESTS_ON_COMPLETE;test_command_timeout: float = 120.0,FOREMAN_TEST_COMMAND_TIMEOUT).start_new_session=True) via SIGTERM→SIGKILL then alwayswait()s — no zombies, no orphaned children; Windows uses terminate/kill + wait.VerificationOutcome(completed/timed_out/launch_failed, returncode, pid, counts, summary) and emits a structuredTEST_RESULTevent — evidence only, never a gate. Never raises for command failures."1 failed, 3 passed"), kept local to this change so it stands alone.Validation
tests/test_runtime_verification.py: success path, failure-first parse, timeout path assertingreturncode is not Noneandos.kill(pid, 0)→ProcessLookupError(fully reaped on POSIX), launch failure, disabled-by-default no-op, structured event emission. Full suite 180 passed,ruff checkclean.