Conversation
|
@codex review Please review exact head The central red control restored the parent production launcher blob and made the mid-poll signal guard fail with pytest exit 1. Restoring the candidate bytes made both focused guards pass. The full signal module, prevention registry tests, bound documentation gate, fixed-point artifact generation, and read-only Ruff checks passed. The full local candidate partition is not established because this host lacks |
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. |
![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: 7f82b71052
ℹ️ 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 LauncherWindow._runtime_callback_is_current(self): | ||
| return False |
There was a problem hiding this comment.
If SIGTERM, SIGINT, or SIGBREAK is delivered after this entry check—for example, while bridge.shutdown() is running—the handler sets _shutdown_requested, but execution then resumes and unconditionally calls bridge.start() at line 6922 before the queued _do_shutdown runs. The mid-poll guard injects the signal before this method begins, so it cannot detect this remaining replacement-after-latch window; recheck runtime authority immediately before starting the replacement and leave the retired bridge settled when shutdown is pending.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| def _signal_handler(signum: int, frame: object) -> None: | ||
| if window._shutdown_requested: | ||
| return | ||
| window._shutdown_requested = True |
There was a problem hiding this comment.
When a signal arrives after _restart_engine's initial shutdown check—such as during _stop_engine, its one-second sleep, or _start_engine—or after the scheduled _do_restart check, this assignment queues shutdown but does not unwind the active callback. Those flows can consequently reach _bridge.start() at lines 5920 or 8162, with the scheduled path potentially spawning a new engine as well, after the latch is already set. Recheck the latch before each spawn/start boundary and settle any partially started replacement instead of attaching its bridge.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
|
@codex review this pull request at exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50a64eff78
ℹ️ 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 LauncherWindow._finish_latched_shutdown(self): | ||
| return | ||
| time.sleep(1) | ||
| if LauncherWindow._finish_latched_shutdown(self): |
There was a problem hiding this comment.
If a signal is delivered after this check returns false but before _start_engine() is called, Python runs the signal handler between those bytecodes, sets _shutdown_requested, and execution still spawns a replacement engine. The post-start check cannot give shutdown ownership until _start_engine() completes its readiness loop, so the latched shutdown may wait several seconds while the new process starts. This is fresh evidence from the new guard placement: the latch must be enforced at the actual spawn boundary rather than only before and after the entire startup operation.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| except Exception as exc: | ||
| LauncherWindow._latch_bridge_watchdog_hold(self, phase="old-settlement", failure=exc) | ||
| return False | ||
| if LauncherWindow._finish_latched_shutdown(self): |
There was a problem hiding this comment.
When a signal lands during the watchdog's bridge.shutdown(), this branch invokes _do_shutdown only after that bridge has closed command admission and stopped its subprocess. For a live acquisition engine, _stop_engine then sends launcher_shutdown through the same retired bridge, receives a lifecycle rejection instead of the required verified-OFF receipt, and routes shutdown into HOLD and forced process reaping. The exact race this branch handles therefore loses the graceful safety shutdown path; preserve a usable shutdown channel or otherwise settle the live engine before retiring the final bridge.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| assert w._restart_pending is False | ||
|
|
||
|
|
||
| def test_scheduled_restart_shutdown_latch_after_bridge_retirement_blocks_engine_spawn() -> None: |
There was a problem hiding this comment.
This guard and the other four newly added latch-boundary tests exist only in the test modules; none of their node names appears in governance/agent_preventions.yaml or its baseline, whose shutdown record still names only the earlier pre-latch, pending-restart, and mid-poll guards. These tests were added for confirmed deterministic restart failures, so leaving them outside the prevention map means a later rename, deselection, or removal will not reopen the corresponding false-green disposition. Add the new nodes to the existing failure-class record with their default-CI partition and evidence state.
AGENTS.md reference: AGENTS.md:L387-L390
Useful? React with 👍 / 👎.
|
@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: 0dc6b5e479
ℹ️ 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 LauncherWindow._runtime_callback_is_current(self): | ||
| raise _EngineStartCancelledForShutdown("launcher shutdown won before engine spawn") | ||
| return subprocess.Popen(command, **kwargs) |
There was a problem hiding this comment.
When SIGTERM/SIGINT arrives after this check but before subprocess.Popen(...) executes, the handler latches shutdown and the call still creates a replacement child; _start_engine then performs the entire readiness transaction, including up to ten half-second attempts, before its caller can observe the latch. The fresh evidence at this head is that _spawn_engine_process merely moves the prior check next to Popen without adding a post-creation check, so publish the returned process owner and immediately divert to shutdown before readiness work when the latch won.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| assert bridge.start_calls == 0 | ||
|
|
||
|
|
||
| def test_shutdown_latched_during_watchdog_retirement_blocks_replacement_start() -> None: |
There was a problem hiding this comment.
At the final head, a repo-wide search of both prevention registry files finds none of the seven newly added deterministic guards covering scheduled/manual restart boundaries, the engine spawn boundary, watchdog retirement, and retired-bridge settlement; the registry change still names only the three earlier signal tests. This is fresh evidence beyond the earlier comment's five-node scope: removing or deselecting this new watchdog-retirement guard would not reopen the false-green prevention, so add these nodes to the applicable record with their default-CI partitions and evidence state.
AGENTS.md reference: AGENTS.md:L387-L390
Useful? React with 👍 / 👎.
| except BaseException: | ||
| try: | ||
| standby.close() | ||
| except BaseException: | ||
| pass |
There was a problem hiding this comment.
When standby startup or its exact-liveness validation fails after creating any runtime owner, close() is allowed to fail—for example, ZmqBridge.close() explicitly raises when subprocess, reply-consumer, queue, or unresolved-command settlement is incomplete—but this handler suppresses that failure and drops the only standby reference before the caller latches HOLD. The resulting process or threads are absent from both _bridge and _watchdog_retired_bridge, so later shutdown cannot settle them and may leave an orphan despite reporting only the original startup error; retain the standby as an owned unsettled bridge whenever cleanup fails.
AGENTS.md reference: AGENTS.md:L470-L471
Useful? React with 👍 / 👎.
| if not LauncherWindow._runtime_callback_is_current(self): | ||
| try: | ||
| shutdown_standby.close() | ||
| finally: | ||
| LauncherWindow._finish_latched_shutdown(self) |
There was a problem hiding this comment.
When a signal arrives after the standby starts but before the old bridge is retired, this new branch closes the healthy standby and immediately runs shutdown through self._bridge, which is the dead, heartbeat-unhealthy, or command-stalled bridge that triggered this watchdog path. In the dead/command-channel-stalled cases, _stop_engine therefore loses the available safe transport and cannot obtain the verified-OFF receipt; this is fresh evidence in the final pre-retirement branch, so promote the standby as the shutdown bridge instead of discarding it before _finish_latched_shutdown.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
|
@codex review Please review exact head c751db0. This head addresses every finding from the previous exact-head review. The launcher now transfers a child returned after a shutdown latch directly into the shutdown path, retains a standby whose cleanup failed, and preserves a healthy standby as the verified-OFF command channel across watchdog retirement. The new boundary guards failed when only launcher.py was restored to parent blob 8f21e3dc356bd0f3bcc42b2a4b93b5cbca762ea7 and passed after byte-exact restoration to bca15476c1753d342595d26612bb869dc9f096e5. Windows and Ubuntu 22.04 each passed the three launcher modules with 118 tests. The registry and baseline include all seven guards and seven false-green counterparts; the derived pair is the final commit and its docs gate passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c751db078e
ℹ️ 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".
| # The child and every credential needed to settle it are now | ||
| # published, so hand ownership straight to shutdown before any | ||
| # readiness reader, pump, or bounded readiness wait can start. | ||
| LauncherWindow._finish_latched_shutdown(self) |
There was a problem hiding this comment.
When the latch lands inside Popen during either manual or scheduled restart, those callers have already run _bridge.shutdown() and will not restart that bridge until _start_engine() returns. Calling _finish_latched_shutdown here consequently makes _stop_engine dispatch launcher_shutdown through a bridge whose command admission is closed, so the newly created child cannot return the required verified-OFF receipt and instead enters HOLD/reaping. Fresh evidence at this head is that the new guard replaces _do_shutdown with a stub that directly marks the fake process dead, so it never exercises this production transport state; hand the child to shutdown through a usable safe-command channel.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| try: | ||
| shutdown_standby.close() | ||
| except Exception as exc: |
There was a problem hiding this comment.
When replacement initialization has already failed and a signal arrives during bridge.shutdown() in the failed-replacement cleanup, the check before that cleanup has already passed; this branch then closes the healthy standby and returns, leaving queued shutdown with the now-stopped self._bridge. The engine therefore loses its remaining verified-OFF command channel. Fresh evidence at this head is the absence of a latch check after failed-replacement settlement—the new watchdog guard injects only during standby validation and initial bridge retirement—so recheck before closing the standby and promote it when shutdown has won.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| except BaseException: | ||
| # Startup failed, but a failed close is still a live owner. | ||
| # Retain it for the ordinary shutdown settlement ladder. | ||
| self._watchdog_retired_bridge = standby |
There was a problem hiding this comment.
When standby startup or validation fails and this close() also fails, the bridge is correctly retained here and _latch_bridge_watchdog_hold marks the transport HOLD, but both operator restart entry points still admit _restart_engine(), which clears _restart_giving_up without checking _bridge_restart_hold or _watchdog_retired_bridge. An operator restart can therefore launch a new engine and restart the primary bridge while the partially started standby process or threads remain unsettled and unregistered; reject manual restart until this retained owner has been settled through the ownership ladder.
AGENTS.md reference: AGENTS.md:L470-L471
Useful? React with 👍 / 👎.
1 participant
Purpose
A signal can latch shutdown while the launcher watchdog is polling a failed bridge. The watchdog could then replace the bridge after shutdown had started. That replacement keeps the process tree alive and makes SIGTERM exceed the shutdown bound.
This change makes bridge replacement return immediately when the runtime callback is no longer current. It also adds guards for a queued restart and for a signal that arrives during the watchdog poll.
Evidence
Exact head:
7f82b7105205af17711c33171066f77d6e1b8a5c.src/cryodaq/launcher.pyto its parent blob.tests/test_launcher_signals.pymodule passed: 20 tests.The full local candidate runner is not established. This host does not have the optional
httpx2dependency, so collection stopped before the remaining partition could run. Hosted CI remains an open gate.Queue check
Other open pull requests also change
src/cryodaq/launcher.py, but their diffs address different defects. Pull request #82 changes spawn readiness. Pull request #112 changes runtime readiness. The other inspected changes do not contain this watchdog latch correction.Merge gates
This pull request is a draft. A clean Codex review at this exact head and green hosted CI are still required. The Ubuntu shutdown and memory measurements must be repeated after merge.