Conversation
Found by review on pull request #95, and it undermines the argument that change rests on. USBTMCTransport is the Keithley 2604B transport, and the Keithley drives the heater. It puts the native VISA session in a separate multiprocessing child so a blocking native call cannot stall the engine event loop. That child is daemonic, and multiprocessing terminates a daemonic child from an atexit handler -- which runs only when the parent exits NORMALLY. An engine that is killed, or that crashes, never runs it, and the child survives. The survivor is not merely untidy. The launcher restarts a dead engine, the replacement connects and commands OFF on every channel -- and the orphan pending write can land AFTER that, leaving the instrument sourcing while the software believes it is off. Two owners of one source is precisely the hazard that makes a crash answerable by a restart at all, so this had to be closed before that reasoning is sound. On Linux the kernel does it: PR_SET_PDEATHSIG asks for SIGKILL when the parent dies, whatever the cause. It carries the classic race -- the parent can die between the fork and the request, so the signal is asked for against a parent that is already gone -- so the parent identity is re-read afterwards and a mismatch exits at once. A child that cannot be bound at all does not run: refusing costs one failed open, which the transport reports, while continuing would risk the orphan. The tests kill a real parent with SIGKILL, the one signal a process cannot handle and therefore the one case atexit can never cover, and require the child to be gone. They are Linux-only and verified on the target.
Found by running the falsification rather than by reading. The first version of this module killed a parent whose child was blocked reading the pipe -- and it passed with the binding REMOVED, because the parent death closes the write end, the read returns end-of-file, and that child leaves by itself. It proved nothing about the guard. The child actually at risk is the one INSIDE a native VISA call: not reading the pipe, unable to see the end-of-file until the call returns, and holding the session that talks to the source. These tests use that child, and they come in a pair: a control that reproduces the orphan without the binding, and the same child bound, which the kernel kills with its parent.
spawn re-imports the main module to rebuild the target, and a "python -c" main module cannot be re-imported: the child died on a traceback before running a line, and the control reported "no orphan" because the child was never alive to become one. The parent is a real file now, and the helper waits and re-checks before killing, so a child that fails to start is reported as that rather than as an absent hazard. Measured properly the hazard is real in every shape tried -- a busy child survives a SIGKILLed parent while sleeping, while spinning in Python, and while blocked in a native call that holds the GIL. The last of those is the shape a real VISA call has. Control and guard now live in one test per shape, so a control that stops reproducing the orphan fails the guard with it. A guard whose hazard cannot be reproduced is a guard nobody can trust.
|
@codex review Head is |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89c12d7cb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| libc = ctypes.CDLL("libc.so.6", use_errno=True) | ||
| if libc.prctl(_PR_SET_PDEATHSIG, signal.SIGKILL, 0, 0, 0) != 0: | ||
| raise OSError(ctypes.get_errno(), "prctl(PR_SET_PDEATHSIG) failed") |
There was a problem hiding this comment.
If the engine dies while the VISA child has submitted or is blocked in a source write, PR_SET_PDEATHSIG only requests SIGKILL; it supplies no receipt that the child and its in-flight USB operation settled before the launcher starts a replacement after its three-second backoff (launcher.py:5651-5658, 5743). The new test permits six seconds for death and exercises only interruptible sleep stand-ins rather than _visa_process_main performing a VISA write, so it does not establish the required ordering. An old transfer can therefore still reach the instrument after the replacement's OFF unless restart/source authority is fenced on descendant/I/O settlement or an independent hardware mechanism.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| usbtmc.os.getppid = lambda: 1 | ||
| usbtmc._bind_lifetime_to_parent() |
There was a problem hiding this comment.
This stub returns 1 on the first getppid() call, so _bind_lifetime_to_parent exits through the earlier expected_parent <= 1 branch without loading libc, invoking prctl, or reaching the post-call identity comparison that this test claims to guard. Deleting lines 476-479 of the production function therefore leaves this test green; make the stub return a valid original parent first and a different parent on the second read, and verify that prctl was reached.
AGENTS.md reference: AGENTS.md:L370-L378
Useful? React with 👍 / 👎.
| finished = subprocess.run([sys.executable, "-c", program], capture_output=True, timeout=30) | ||
| assert b"KEPT RUNNING" not in finished.stdout, ( | ||
| "a child that could not bind its lifetime to its parent must exit, not continue" |
There was a problem hiding this comment.
When this probe fails to import the module, construct the ctypes double, or execute the helper for any unrelated reason, KEPT RUNNING is also absent and the assertion passes; the parent-race probe at line 214 has the same false-green shape. Require an explicit marker proving the intended prctl path ran and validate the expected exit status so a traceback or premature interpreter failure cannot satisfy these safety guards.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| time.sleep(1.0) | ||
| assert _alive(child_pid), f"the child died before the parent was killed; stderr={parent.stderr.read()[:600]!r}" |
There was a problem hiding this comment.
If a regression makes the spawned child die after its PID is reported, evaluating this assertion message calls parent.stderr.read() while the parent is still sleeping for 600 seconds and still owns the pipe, so the read blocks and the finally cleanup cannot run. The guard then hangs for roughly ten minutes instead of reporting the startup failure; terminate/wait for the parent before reading stderr, or use a bounded/nonblocking diagnostic read.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
| expected_parent = os.getppid() | ||
| if expected_parent <= 1: | ||
| # Already reparented: the parent died before we got here. Nothing can be bound. | ||
| os._exit(0) |
There was a problem hiding this comment.
If the engine dies before the spawned child executes this first getppid(), and a launcher or service supervisor is a Linux child subreaper, the call returns that surviving ancestor's PID rather than the dead engine's PID. The child then successfully binds PDEATHSIG to the wrong process, the second identity read matches, and the VISA owner can outlive the engine exactly as before. Capture os.getpid() in the engine before Process.start() and pass that expected PID to the child so reparenting before its first instruction is detected.
AGENTS.md reference: AGENTS.md:L470-L471
Useful? React with 👍 / 👎.
Closes the remaining review finding on pull request #95. The other one, about engine descendants outliving the engine, is closed by #98 -- it is the driver's contract rather than the launcher's, and the Keithley's VISA child is the thing that had to be bound. _EngineShutdownWorker is a QThread whose run() is blocked inside send_command on the very bridge recovery is about to shut down. When a replacement exited while that reply was still outstanding, _stop_engine raised after its grace period, the exit was reclassified as observed, and retiring the incarnation dropped the only retained reference to a thread that was still running. Qt is then free to destroy it -- "QThread: Destroyed while thread is still running" -- which stops the launcher instead of recovering it. That is the opposite of what this branch is for. The worker is now waited out first, bounded twice over: its command carries its own timeout, and the wait has its own so a wedged worker cannot stall the Qt main thread. If it finishes, the reference is cleared and the identity retired as before. If it does not, the reference is KEPT, the identity is NOT retired, and the operator sees a HOLD saying so -- an owner that cannot be settled is the one thing that must still hold. The backoff fixture had to say something out loud. It is a MagicMock, which answers every attribute, so it was auto-vivifying a shutdown worker that was permanently "still running" -- a launcher that has not dispatched a shutdown has none. Three tests failed on that invented worker rather than on any production behaviour. Dropping the reference again reddens exactly the new test.
…e binder Four review findings on pull request #98. The child must not decide which parent it belongs to. If the engine dies before the child runs its first instruction, and any ancestor is a Linux child subreaper, getppid() answers with that surviving ancestor -- so the child bound PDEATHSIG to the wrong process, the second identity read agreed with itself, and the VISA owner outlived the engine exactly as before. The soak runner IS such a subreaper, so this was not hypothetical. The engine captures its own pid before Process.start() and hands it over. Three false-green shapes in the tests, all the same kind: the absence of a marker is not proof that the intended path ran. - the two refusal probes passed if the module failed to import, the ctypes double was wrong, or anything unrelated failed, because the marker was absent either way. Every probe now prints one immediately before calling the binder, and the exit status is checked as well. - the parent-race probe returned 1 from the FIRST getppid(), so the function left through the already-reparented branch without loading libc or calling prctl at all -- deleting the post-call comparison left it green. It answers with a real parent first and a different one after now. - the diagnostic on a failed start read the parent stderr while the parent was still sleeping for ten minutes and still owned the pipe, so a clear startup failure became a ten-minute hang with no message. The parent is killed and reaped before its stderr is read.
|
@codex review Head is Closed: the engine captures its own pid before NOT closed, and I do not think it can be closed in software: your point that The project's own laboratory checklist already names the closing element: an independent latching de-energize-to-trip cutout, section A8e, currently marked NOT PASSED, with the reference manual quoted as saying the SMU's own digital output is unsuitable for safety circuits. That is hardware, and it is the owner's to build. I would rather record this residual honestly here than imply software closed it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ece319c23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| os._exit(0) | ||
|
|
||
|
|
||
| def _visa_process_main(connection: Any, expected_parent: int = 0) -> None: |
There was a problem hiding this comment.
On Linux, five existing protocol tests call _visa_process_main(connection) with one argument, so this default supplies 0, _bind_lifetime_to_parent reaches os._exit(0), and the entire pytest process exits successfully before those tests or any later tests run. I reproduced this with the combined new/protocol modules: pytest collected 69 items, stopped at the first direct worker test, emitted no session summary, and returned status 0; thus ordinary local runs can falsely report success, while the receipt-enforcing candidate runner should fail for a missing population receipt. Make the parent PID mandatory and adapt the in-process tests to exercise the worker loop without invoking a process-fatal binder.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
| one source is the exact hazard the ownership design exists to prevent, and it is the reason | ||
| an engine crash may be answered with a restart at all. | ||
|
|
||
| WHICH CHILD IS AT RISK, and TWO measurement mistakes made on the way to knowing it. |
There was a problem hiding this comment.
This module explicitly records two confirmed measurement mistakes, plus the underlying source-owner survival defect, but the slice adds no corresponding runtime or false-green entries to governance/agent_preventions.yaml; the existing USBTMC-CLOSE-SETTLEMENT-001B record is limited to cancellation/incomplete-close ownership and does not cover abrupt parent death or either invalid probe shape. Consequently these acknowledged failures have no stable IDs, default-CI guard bindings, or red/green evidence in the repository's mandatory prevention map, so this corrective disposition cannot close until those obligations are registered (or an existing class is explicitly strengthened).
AGENTS.md reference: AGENTS.md:L361-L369
Useful? React with 👍 / 👎.
Continuous integration failed on a suite where every one of 693 tests passed, with "candidate pytest invocation 2 emitted 0 population receipts instead of one". The control case leaks a process on purpose, because that IS the hazard. But the leaked process inherited pytest's own standard output and error, so it kept those pipes open after pytest had exited and the runner could not close its accounting. What is under test is a process outliving its parent, not the pipes it happens to hold, so the child now redirects its own standard streams to the null device before it does anything else. It is also reaped rather than merely signalled. It is nobody's child any more, so nothing will wait on it, and leaving it running is how one test's deliberate leak becomes the next test's environment.
|
@codex review Head is |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
One review finding, and it is the shape where a safety mechanism turns on the person trying to test it. FIVE PROTOCOL TESTS CALL `_visa_process_main(connection)` WITH ONE ARGUMENT. With the parent-death binding at the top of that function and `expected_parent` defaulting to 0, those tests bound their own lifetime to a parent that is not the one they have -- on Linux that is a signal to the test process itself. The entry point now REQUIRES `expected_parent`, so no caller can reach the binding by accident, and the request loop is split into `_visa_worker_loop`, which the direct tests call instead. The binding still happens before any VISA handle exists, which is the property it was written for. Authored by a worker lane, reviewed by a second model, and verified here rather than taken on either one's word: LF endings, ruff clean, and both properties falsified -- restoring the default parent, and moving the binding after the loop, each turn exactly one test red. `tests/drivers/test_usbtmc_process_protocol.py` and `tests/drivers/test_the_source_child_dies_with_its_engine.py` are green at 65 passed, 5 skipped.
|
@codex review Head is Five protocol tests call The entry point now requires Authored by a worker lane, reviewed by a second model, and verified here rather than taken on either one's word: LF endings, ruff clean, and both properties falsified — restoring the default parent, and moving the binding after the loop, each turn exactly one test red.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e14ff4a814
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| try: | ||
| assert parent.stdout is not None | ||
| line = parent.stdout.readline().decode().strip() |
There was a problem hiding this comment.
When the spawned parent remains alive but stalls in Process.start() before printing the child PID, this blocking readline() has no timeout, so the Ubuntu remaining process test hangs indefinitely and never reaches the finally cleanup. Read the startup line with a bounded poll/selector or otherwise terminate the parent after a fixed startup deadline so this safety regression remains deterministic.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
| time.sleep(1.0) | ||
| if not _alive(child_pid): | ||
| raise AssertionError(_diagnose("the child died before the parent was killed")) |
There was a problem hiding this comment.
When child startup takes longer than this fixed one-second sleep, _alive(child_pid) proves only that the interpreter exists, not that _bind_lifetime_to_parent() reached prctl. The parent can then be killed first; even with the prctl call deleted, the late binder exits through its initial parent-identity mismatch while the unbound control survives, so the real parent-death guard remains green without exercising PDEATHSIG. Require a bounded readiness marker emitted after binding (and at the equivalent point in the control) before killing the parent.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
Two review findings on the same regression, both correct. The child PID was read with an unbounded readline(), so a parent that stalled inside Process.start() hung the Ubuntu run forever and never reached cleanup. The read is now bounded by a selector with a fixed deadline. The one-second sleep before killing the parent proved only that an interpreter existed, not that _bind_lifetime_to_parent had reached prctl. With the prctl call deleted, a late binder still exited through its initial parent-identity mismatch while the unbound control survived -- so the guard stayed green without ever exercising PDEATHSIG. The child now sends a READY marker after binding, and at the equivalent point in the no-binding control, and the parent is killed only after that marker arrives. Stated plainly rather than glossed: these guards SKIP on Windows, so this machine can show only that the module imports and the one portable test passes. The evidence that matters comes from the Ubuntu run.
|
@codex review this pull request at head Both findings are answered. Bounded startup read. The unbounded Readiness before the kill. The fixed one-second sleep is gone. The child sends a READY One thing said plainly instead of glossed: these guards SKIP on Windows, where this work |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # docs/architecture-montana-important.svg # docs/current_candidate_metrics.md
|
@codex review this pull request at head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d8c04ae23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not sys.platform.startswith("linux"): | ||
| # Elsewhere the daemonic flag remains the only mechanism, and an abruptly dead | ||
| # parent can still leave this process behind. The laboratory target is Ubuntu. | ||
| return |
There was a problem hiding this comment.
On Windows—the real USBTMC/VISA path prescribed by docs/lab_verification_checklist.md:249-250—this branch returns before installing any lifetime coupling, even though the comment acknowledges that an abruptly terminated engine can leave the source-owning child alive. A later manual or supervised engine start can therefore create a second VISA owner while the orphan finishes a source write; either provide equivalent Windows process-tree ownership (such as a kill-on-close Job Object) or refuse non-mock VISA startup on platforms without it.
AGENTS.md reference: AGENTS.md:L470-L471
Useful? React with 👍 / 👎.
|
@codex review this pull request at head |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Integrates #98 as block C. Zero product conflicts with the candidate -- it touches only src/cryodaq/drivers/transport/usbtmc.py and its two test files. The property: no Keithley transport subprocess may outlive the Engine that owns it. On Linux this uses the kernel's own guarantee -- prctl(PR_SET_PDEATHSIG, SIGKILL) in the child, so the kernel kills it when the parent dies, rather than trusting a cleanup path that a crashed parent never reaches. If the call fails it raises rather than continuing silently. Why the campaign requires this before any crash test with real hardware: two processes able to command the same source is the single-actuator-owner invariant broken, and an orphan holding the instrument after its Engine died is how that happens in practice. Until this is proven with a real subprocess on the target platform, Engine crash/restart stays outside the candidate's physical claim and is exercised only against a dummy load with independent manual protection. Reversion control on this candidate: 100 passed, 2 skipped with the change; production reverted, 34 fail. The two skips are environment-gated, not silenced assertions. Block C. Semantic delta only; the merge's governance baseline was reverted to master and reconciles once at freeze.
|
The production fix on this branch is already in master, under But the fix is narrower than this pull request's title, and the gap is measurable. What master bindsOne child: the USBTMC transport child, with a real kernel binding — Linux What nothing bindsThe launcher's own assistant child. At The measurementTaken 2026-08-30 against master Measured separately the same morning, from a genuinely clean slate with every process killed between attempts: the launcher fails to reach engine readiness about one time in three. A failed construction is exactly the path that strands the assistant, because the graceful sentinel is never written. So a laboratory week that restarts a dozen times accumulates roughly a gigabyte of abandoned children. That is the memory-growth criterion directly, and it is not what master fixes. What is being doneThe gap is being closed on this branch rather than in a new pull request, because it is the same subject one scope wider. The work is to bind the launcher's assistant child the way the transport child is bound — reusing the two mechanisms that already exist rather than writing a third copy — and to cover the failure paths, not only clean shutdown: a launcher that fails during construction, one that is killed, and one whose engine dies under it. The guard must start the real thing and assert no child survives; a test that simulates the process topology cannot see this. The eleven multiprocessing children are in scope for the answer, not necessarily for the fix: whether a binding on the parent covers them transitively will be stated from evidence or stated as unknown, not assumed. The branch's own remaining delta against master is otherwise governance records and the regenerated derived pair. Written with AI assistance; the process measurements above were taken and read directly. |
test1card
changed the title
|
This branch has been replaced, so please review it as a new change rather than as a revision. Three things I would most like challenged:
Controls at this head, each asserting its mutation applied before any result was read, and each
No test was weakened: both test files are pure additions, 477 insertions and 0 deletions. It does not claim to fix everything. Local at this head: new tests 67 passed; Written with AI assistance. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6445f308
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # bounded report children, assignment failure is fail-closed | ||
| # below and the retained job handle remains launcher-owned for | ||
| # exactly as long as the assistant incarnation. | ||
| parent_job = create_windows_kill_on_close_job(process) |
There was a problem hiding this comment.
On Windows, if the launcher exits after Popen returns but before this assignment completes, the already-running assistant has never joined the kill-on-close Job, so it survives exactly the launcher death this change is intended to cover; it may also create descendants during that interval that are outside the subsequently assigned tree. The two Windows tests mock the post-spawn assignment and therefore cannot detect this window; the child must not run until its Job ownership is established.
AGENTS.md reference: AGENTS.md:L470-L471
Useful? React with 👍 / 👎.
| deadline = time.monotonic() + 15.0 | ||
| while time.monotonic() < deadline and not readiness_path.is_dir(): | ||
| if _identity_exited(assistant_pidfd): | ||
| stderr = b"" if parent.stderr is None else parent.stderr.read(4000) |
There was a problem hiding this comment.
When the assistant exits before creating the readiness directory while the launcher harness remains alive, this bounded-looking diagnostic actually blocks because BufferedReader.read(4000) waits for 4,000 bytes or EOF, but the harness retains the stderr writer in its infinite loop. Fresh evidence in this replacement branch is that the new harness repeats the previously identified live-parent stderr-read shape, so an early bootstrap regression can hang the test instead of entering its bounded cleanup; terminate the parent first or use a nonblocking/bounded diagnostic read.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
| parent = subprocess.Popen( | ||
| [sys.executable, "-B", str(harness), mode], | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| env=env, | ||
| ) |
There was a problem hiding this comment.
If Popen raises after _set_subreaper(True)—for example under process or descriptor exhaustion—execution never enters the try/finally below, so pytest remains a child subreaper for the rest of the run. Fresh evidence in this replacement harness is that process-global state is enabled at line 233 while the new launcher creation remains outside the cleanup scope; later process tests can consequently adopt unrelated descendants and produce leaks or topology-dependent false results. Put process creation inside the restoration boundary.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
| if mode == "engine-death": | ||
| engine_pid = report["engine_pid"] | ||
| assert type(engine_pid) is int and engine_pid > 1 | ||
| engine_pidfd = _pidfd_open(engine_pid) |
There was a problem hiding this comment.
In engine-death mode, the harness has already spawned and reported the 600-second engine, but its PID is not captured until after assistant pidfd setup and readiness. If either of those earlier operations fails or readiness times out, finally kills only the launcher and assistant; the sibling engine is adopted by the test's subreaper and left sleeping, then potentially left as an unreaped child. Fresh evidence here is the new harness report already contains engine_pid, so capture and pin it immediately and settle it unconditionally during cleanup.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
test1card
changed the title
|
@codex review this pull request at exact head This head closes the Windows pre-Job execution race, the blocking early-stderr read, subreaper restoration on construction failure, and engine cleanup after readiness failure. The coordinator reproduced all three new production controls red against the exact pre-fix blobs, restored them byte-for-byte, and ran the Windows, governance, documentation, lint, and format gates stated in the pull-request body. |
|
Ubuntu 22.04 verification for exact head The coordinator fetched
Result: 103 passed. This supplements the native-Windows result in the pull-request body. It does not claim physical hardware coverage. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b8dd261ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| monkeypatch.setattr(module.subprocess, "Popen", popen) | ||
| monkeypatch.setattr(module, "create_windows_kill_on_close_job", create_job) | ||
| monkeypatch.setattr(module, "resume_windows_process", resume, raising=False) |
There was a problem hiding this comment.
On native Windows, these registered guards still replace Popen, Job assignment, and resume with Python doubles, so removing JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE or breaking the real AssignProcessToJobObject call leaves every Windows assistant-lifetime guard green. Fresh evidence at this head is that the prevention registry adds only these mocked Windows nodes, while no test kills a real launcher and observes its real assistant tree exit; add a production-boundary Windows test that exercises the actual spawn, assignment, resume, and launcher-death path.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| import psutil | ||
|
|
||
| psutil.Process(process.pid).resume() |
There was a problem hiding this comment.
In a Windows installation resolved from the project's base package metadata rather than the repository's developer lock, psutil is absent because pyproject.toml declares it only in the dev extra and explicitly describes it as dev-only. Every assistant start now reaches this import after creating the child suspended and assigning its Job, so it raises ModuleNotFoundError, reports a post-spawn construction failure, and the assistant never runs; use the existing Windows handle/API approach or declare psutil as a runtime dependency.
Useful? React with 👍 / 👎.
| - node: tests/launcher/test_assistant_parent_lifetime.py::test_early_exit_diagnostic_is_bounded_while_writer_remains_open | ||
| ci_partition: remaining | ||
| - node: tests/launcher/test_assistant_parent_lifetime.py::test_parent_creation_failure_restores_subreaper | ||
| ci_partition: remaining | ||
| - node: tests/launcher/test_assistant_parent_lifetime.py::test_engine_identity_is_cleaned_if_readiness_fails_after_report |
There was a problem hiding this comment.
Fresh evidence at this head is that the blocking diagnostic, construction-failure restoration, and readiness-cleanup regressions are added only as guards beneath the existing runtime record, while false_green_pairs still contains only the earlier binder-deletion escape for this prevention. Because each newly reproduced defect remained undetected by the previously green suite, each coverage escape needs its own stable false-green ID, default-CI guard binding, and red/green evidence before this disposition can close.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
| if mode == "engine-death": | ||
| engine_pid = report["engine_pid"] | ||
| assert type(engine_pid) is int and engine_pid > 1 | ||
| engine_pidfd = _pidfd_open(engine_pid) |
There was a problem hiding this comment.
If opening the reported engine's pidfd fails here—for example because the process hits EMFILE or pidfds become unavailable after preflight—engine_pid is recorded but engine_pidfd remains None. The finally block consequently kills only the harness parent; its 600-second engine is adopted by the test subreaper without ever being signalled, and _reap_adopted_child times out while leaving that process alive. Establish an unconditional parent-owned engine cleanup path before this fallible open, or otherwise settle the reported identity when pidfd acquisition fails.
AGENTS.md reference: AGENTS.md:L291-L298
Useful? React with 👍 / 👎.
|
@codex review Please review exact head This head answers the four findings from the prior review:
Independent coordinator verification on Windows passed 71 tests with four expected Linux skips. The prevention registry tests, bound documentation gate, fixed-point derived artifacts, Ruff check, Ruff format check, and diff check passed. The local hosted-equivalent remaining runner is not established on this host. Its first invocation stopped during collection because |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b673aafbba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def test_windows_resume_path_has_no_dev_only_psutil_import() -> None: | ||
| import cryodaq.process_lifetime as module | ||
|
|
||
| assert "psutil" not in module.resume_windows_process.__code__.co_names |
There was a problem hiding this comment.
This structural check only inspects resume_windows_process itself, so it remains green if psutil is imported at module scope, through _load_windows_ntdll, or via importlib; the default test environment installs the locked development dependency, allowing the native launcher guard to remain green too, while a base-only Windows installation would still fail before resuming the assistant. Exercise the production start/resume path with psutil imports explicitly blocked or in a base-only environment instead of relying on co_names.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| # This is before logging, configuration, sockets, renderers, or optional | ||
| # LLM imports can acquire runtime ownership. A child first scheduled after | ||
| # launcher death exits on the captured-parent mismatch. | ||
| bind_child_lifetime_from_environment() |
There was a problem hiding this comment.
On Linux this call runs only after Python has imported yaml, report_coordinator, report_process, and their transitive modules, so if the launcher dies while one of those imports is stalled, the child has not installed PDEATHSIG and can survive at the stalled pre-main() instruction indefinitely. The lifecycle guard waits for post-bootstrap readiness before killing the launcher and therefore cannot observe this window; use a minimal pre-import entrypoint that consumes the parent grant and binds lifetime before loading assistant_bootstrap or the frozen assistant dispatcher.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| assert parent.stdout is not None | ||
| parent.stdin.write(b"START\n") | ||
| parent.stdin.flush() | ||
| report = json.loads(_read_line_from_windows_pipe(parent.stdout, timeout=15.0)) |
There was a problem hiding this comment.
The native Windows guard receives this report only after _start_assistant() has completed assignment and resume, so deleting CREATE_SUSPENDED still lets the assistant enter its Job quickly, reach readiness, and die when the launcher is killed; the test remains green while restoring the pre-assignment execution/descendant race. The separate fake-Popen test merely interprets the flag value and does not exercise this OS boundary, so add a native synchronization point that delays real Job assignment and proves the real assistant cannot execute before assignment completes.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| if parent is not None and not parent_settled: | ||
| parent.kill() | ||
| parent.wait(timeout=10.0) |
There was a problem hiding this comment.
If the Windows harness parent’s kill() or bounded wait() raises here, execution leaves the finally block immediately and skips the subsequent exact assistant termination, assistant-handle close, and stream cleanup. Under process-handle failure or timeout, this native guard can therefore leak both the launcher and the production assistant it started; collect the parent cleanup error while still attempting every assistant and handle settlement step, then propagate the preserved failure afterward.
AGENTS.md reference: AGENTS.md:L291-L298
Useful? React with 👍 / 👎.
| self._kernel32.CloseHandle(self._handle) | ||
| self._handle = None |
There was a problem hiding this comment.
When CloseHandle returns false, this method still discards _handle and reports success. If the assistant root or report renderer has exited while a Job-owned descendant remains, that failed close is the operation that was supposed to kill the descendant; losing the only Job handle lets shutdown complete and a replacement start while the old tree remains live. Check the native return value, preserve the exact handle on failure, and raise so the existing settlement path can retry.
AGENTS.md reference: AGENTS.md:L470-L471
Useful? React with 👍 / 👎.
| selector.register(stream, selectors.EVENT_READ) | ||
| if not selector.select(timeout): | ||
| raise AssertionError("launcher harness did not publish its process identities") | ||
| line = stream.readline() |
There was a problem hiding this comment.
The selector bounds only the arrival of the first byte; once a live harness writes any partial JSON without a newline, BufferedReader.readline() blocks indefinitely while the writer remains open, so the lifecycle test never enters its process, pidfd, or subreaper cleanup. Read incrementally under one deadline, as the hardened USBTMC lifecycle harness does, so a partial startup frame becomes a bounded failure rather than a hung remaining partition.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
| if parent is not None: | ||
| if parent.stdin is not None: | ||
| parent.stdin.close() | ||
| if parent.stdout is not None: | ||
| parent.stdout.close() | ||
| if parent.stderr is not None: | ||
| parent.stderr.close() |
There was a problem hiding this comment.
If closing any one of these parent streams raises, the remaining closes, adopted-child reaps, pidfd closes, and _set_subreaper(previous_subreaper) are all skipped because these operations are not part of the attempt-all error collection used immediately above and below. A broken-pipe or descriptor-close failure can consequently leave pytest as a process-global child subreaper and corrupt every later process test; capture each close failure independently and restore subreaper state before propagating the first error.
AGENTS.md reference: AGENTS.md:L291-L298
Useful? React with 👍 / 👎.
|
@codex review this pull request at exact head |
There was a problem hiding this comment.
💡 Codex Review
cryodaq/src/cryodaq/launcher.py
Line 8335 in 1d1da7a
Keep recurring status workers parentless
Passing parent=self makes every completed safety worker a QObject child of the long-lived launcher window, and the identical annunciation construction below does the same. The registry releases terminal workers without calling deleteLater, while the attributes retain only the newest pair, so Qt retains two historical QThreads on every three-second health tick until the window is destroyed—about 2,400 objects per hour. Restore the parentless ownership model or explicitly dispose terminal workers, and retain the deleted bounded-retention regression.
AGENTS.md reference: AGENTS.md:L540-L542
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not self._kernel32.CloseHandle(handle): | ||
| # The Job remains the only kill-on-close owner. Retain the | ||
| # exact handle so launcher shutdown can retry settlement. | ||
| import ctypes | ||
|
|
||
| raise OSError(ctypes.get_last_error(), "CloseHandle failed for kill-on-close Job") |
There was a problem hiding this comment.
When a Windows report renderer exits after leaving a descendant and this first CloseHandle attempt fails, ReportProcessRunner._run_process calls close() only once on its ordinary-exit path and then unwinds with windows_job held only in that local frame. Fresh evidence at this head is that retaining _handle here gives the launcher a retry because it stores the Job on self, but the report path still has no callable retry path, so the descendant can survive after the exception is propagated. Persist that owner or perform a bounded retry before the report frame releases it.
AGENTS.md reference: AGENTS.md:L470-L471
Useful? React with 👍 / 👎.
| time.sleep(0.01) | ||
| assert assignment_pid_path.is_file(), "launcher never reached delayed Job assignment" | ||
| assistant_pid = int(assignment_pid_path.read_text(encoding="ascii")) | ||
| kernel32, assistant_handle = _open_windows_process_identity(assistant_pid) |
There was a problem hiding this comment.
If OpenProcess fails here after the atomic PID marker is published—for example under handle exhaustion or access denial—the harness parent is still blocked before Job assignment and assistant_handle remains None. The finally block then kills the parent but skips assistant termination, leaving the unassigned suspended assistant (or a running assistant under the regression being tested) alive indefinitely; establish unconditional cleanup authority before this fallible open or release the parent into Job assignment during cleanup.
AGENTS.md reference: AGENTS.md:L291-L298
Useful? React with 👍 / 👎.
1 participant
Purpose
This pull request binds the launcher assistant process to the launcher lifetime on Ubuntu and Windows.
The original USBTMC source-child fix is already in
master. This branch updates the existing pull request instead of opening a duplicate.Mechanism
A shared
process_lifetime.pymodule now owns the operating-system bindings.PR_SET_PDEATHSIGbefore optional runtime work starts. It also checks the captured parent identity before and afterprctl.The USBTMC and report-process deletions are moves into the shared module. The branch retains their old private import names.
Scope limit
PR_SET_PDEATHSIGis not transitive. This change binds the launcher assistant and preserves the USBTMC binding. It does not cover the other multiprocessing children.Review corrections
This head closes four independent review findings:
The native-Windows guard uses
SIGTERMwhereSIGKILLis unavailable. Linux still uses the realSIGKILLboundary.Verification for this head
The coordinator replaced the corrected production blobs with their exact pre-fix blobs. The three new guards failed. The coordinator then restored the corrected blobs byte-for-byte.
Native Windows passed the three affected modules: 95 passed and 8 skipped. The full governance set passed 308 tests. The documentation freshness gate passed 68 tests after the generated pair became the last commit. Read-only Ruff check and format check passed for all five changed Python files.
Ubuntu hosted checks remain pending. This pull request stays a draft until the exact-head review is clean.
Written with AI assistance.